From 7064053ffea14d6222e4a81218923fb2a1f55c1e Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Wed, 13 Nov 2024 15:49:40 +0100 Subject: [PATCH] fix: Avoid dumping unique_constraint when attisdropped (#349) Fixes #347 * fix: ignore unique constraints in favor of indexes * fix: do not support unique_constraints * clarity refactor * ignore new failing test --- .../cockroachdb_adapter.rb | 48 +++++++++-------- .../cases/migration/unique_constraint_test.rb | 51 ------------------- test/cases/schema_dumper_test.rb | 34 +++++++++---- .../PostgreSQLAdapterTest.rb | 2 + .../Migration/UniqueConstraintTest.rb | 16 ------ test/excludes/SchemaDumperTest.rb | 1 - 6 files changed, 55 insertions(+), 97 deletions(-) delete mode 100644 test/cases/migration/unique_constraint_test.rb delete mode 100644 test/excludes/ActiveRecord/Migration/UniqueConstraintTest.rb diff --git a/lib/active_record/connection_adapters/cockroachdb_adapter.rb b/lib/active_record/connection_adapters/cockroachdb_adapter.rb index 5b42506a..0ba79841 100644 --- a/lib/active_record/connection_adapters/cockroachdb_adapter.rb +++ b/lib/active_record/connection_adapters/cockroachdb_adapter.rb @@ -181,6 +181,17 @@ def supports_exclusion_constraints? false end + # OVERRIDE: UNIQUE CONSTRAINTS will create indexes anyway, so we only consider + # then as indexes. + # See https://github.com/cockroachdb/activerecord-cockroachdb-adapter/issues/347. + # See https://www.cockroachlabs.com/docs/stable/unique. + # + # NOTE: support is actually partial, one can still use the `#unique_constraints` + # method to get the unique constraints. + def supports_unique_constraints? + false + end + def supports_expression_index? # Expression indexes are partially supported by CockroachDB v21.2, # but activerecord requires "ON CONFLICT expression" support. @@ -393,32 +404,29 @@ def column_definitions(table_name) # have [] appended to the end of it. re = /\A(?:geometry|geography|interval|numeric)/ - # 0: attname - # 1: type - # 2: default - # 3: attnotnull - # 4: atttypid - # 5: atttypmod - # 6: collname - # 7: comment - # 8: attidentity - # 9: attgenerated - # 10: is_hidden + f_attname = 0 + f_type = 1 + # f_default = 2 + # f_attnotnull = 3 + # f_atttypid = 4 + # f_atttypmod = 5 + # f_collname = 6 + f_comment = 7 + # f_attidentity = 8 + # f_attgenerated = 9 + f_is_hidden = 10 fields.map do |field| - dtype = field[1] - field[1] = crdb_fields[field[0]][2].downcase if re.match(dtype) - field[7] = crdb_fields[field[0]][1]&.gsub!(/^\'|\'?$/, '') - field[10] = true if crdb_fields[field[0]][3] + 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]&.gsub!(/^\'|\'?$/, '') + field[f_is_hidden] = true if crdb_fields[field[f_attname]][3] field end fields.delete_if do |field| # Don't include rowid column if it is hidden and the primary key # is not defined (meaning CRDB implicitly created it). - if field[0] == CockroachDBAdapter::DEFAULT_PRIMARY_KEY - field[10] && !primary_key(table_name) - else - false # Keep this entry. - end + field[f_attname] == CockroachDBAdapter::DEFAULT_PRIMARY_KEY && + field[f_is_hidden] && !primary_key(table_name) end end diff --git a/test/cases/migration/unique_constraint_test.rb b/test/cases/migration/unique_constraint_test.rb deleted file mode 100644 index 401d1e15..00000000 --- a/test/cases/migration/unique_constraint_test.rb +++ /dev/null @@ -1,51 +0,0 @@ -# frozen_string_literal: true - -require "cases/helper_cockroachdb" -require "support/schema_dumping_helper" - - -module ActiveRecord - module Cockroach - class Migration - class UniqueConstraintTest < ActiveRecord::TestCase - - setup do - @connection = ActiveRecord::Base.lease_connection - @connection.create_table "sections", force: true do |t| - t.integer "position", null: false - end - end - - teardown do - @connection.drop_table "sections", if_exists: true - end - - def test_unique_constraints - unique_constraints = @connection.unique_constraints("test_unique_constraints") - - expected_constraints = [ - { - name: "test_unique_constraints_position_1", - column: ["position_1"] - }, { - name: "test_unique_constraints_position_2", - column: ["position_2"] - }, { - name: "test_unique_constraints_position_3", - column: ["position_3"] - } - ] - - assert_equal expected_constraints.size, unique_constraints.size - - expected_constraints.each do |expected_constraint| - constraint = unique_constraints.find { |constraint| constraint.name == expected_constraint[:name] } - assert_equal "test_unique_constraints", constraint.table_name - assert_equal expected_constraint[:name], constraint.name - assert_equal expected_constraint[:column], constraint.column - end - end - end - end - end -end diff --git a/test/cases/schema_dumper_test.rb b/test/cases/schema_dumper_test.rb index 6c4110d9..9b28e141 100644 --- a/test/cases/schema_dumper_test.rb +++ b/test/cases/schema_dumper_test.rb @@ -23,15 +23,31 @@ def perform_schema_dump dump_all_table_schema [] end - # OVERRIDE: we removed the 'deferrable' part in `assert_match` - def test_schema_dumps_unique_constraints - output = dump_table_schema("test_unique_constraints") - constraint_definitions = output.split(/\n/).grep(/t\.unique_constraint/) - - assert_equal 3, constraint_definitions.size - assert_match 't.unique_constraint ["position_1"], name: "test_unique_constraints_position_1"', output - assert_match 't.unique_constraint ["position_2"], name: "test_unique_constraints_position_2"', output - assert_match 't.unique_constraint ["position_3"], name: "test_unique_constraints_position_3"', output + # See https://github.com/cockroachdb/activerecord-cockroachdb-adapter/issues/347 + def test_dump_index_rather_than_unique_constraints + ActiveRecord::Base.with_connection do |conn| + conn.create_table :payments, force: true do |t| + t.text "name" + t.integer "value" + t.unique_constraint ["name", "value"], name: "as_unique_constraint" # Will be ignored + t.index "lower(name::STRING) ASC", name: "simple_unique", unique: true + t.index "name", name: "unique_with_where", where: "name IS NOT NULL", unique: true + end + end + + stream = StringIO.new + ActiveRecord::Base.connection_pool.with_connection do |conn| + dumper = conn.create_schema_dumper({}) + dumper.send(:table, "payments", stream) + end + stream.rewind + index_lines = stream.each_line.select { _1[/simple_unique|unique_with_where|as_unique_constraint/] } + assert_equal 2, index_lines.size + index_lines.each do |line| + assert_match(/t.index/, line) + end + ensure + ActiveRecord::Base.with_connection { _1.drop_table :payments, if_exists: true } end def test_schema_dump_with_timestamptz_datetime_format diff --git a/test/excludes/ActiveRecord/ConnectionAdapters/PostgreSQLAdapterTest.rb b/test/excludes/ActiveRecord/ConnectionAdapters/PostgreSQLAdapterTest.rb index 386c0948..4ea833bd 100644 --- a/test/excludes/ActiveRecord/ConnectionAdapters/PostgreSQLAdapterTest.rb +++ b/test/excludes/ActiveRecord/ConnectionAdapters/PostgreSQLAdapterTest.rb @@ -31,3 +31,5 @@ exclude :test_warnings_behaviour_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 + +exclude :test_translate_no_connection_exception_to_not_established, "CRDB doesn't implement pg_terminate_backend()" diff --git a/test/excludes/ActiveRecord/Migration/UniqueConstraintTest.rb b/test/excludes/ActiveRecord/Migration/UniqueConstraintTest.rb deleted file mode 100644 index b2bb846c..00000000 --- a/test/excludes/ActiveRecord/Migration/UniqueConstraintTest.rb +++ /dev/null @@ -1,16 +0,0 @@ -exclude :test_unique_constraints, "Removed deferrable part from constraints." - -no_deferrable = "CRDB doesn't support DEFERRABLE constraints" -exclude :test_add_unique_constraint_with_deferrable_deferred, no_deferrable -exclude :test_add_unique_constraint_with_deferrable_immediate, no_deferrable -exclude :test_added_deferrable_initially_immediate_unique_constraint, no_deferrable - -no_using_index = "CRDB doesn't support USING INDEX" -exclude :test_add_unique_constraint_with_name_and_using_index, no_using_index -exclude :test_add_unique_constraint_with_only_using_index, no_using_index - -no_remove_unique_constraint = "CRDB doesn't support " \ - "ALTER TABLE DROP CONSTRAINT. There may be an altenative, see " \ - "https://github.com/cockroachdb/activerecord-cockroachdb-adapter/issues/304" -exclude :test_remove_unique_constraint, no_remove_unique_constraint -exclude :test_remove_unique_constraint_by_column, no_remove_unique_constraint diff --git a/test/excludes/SchemaDumperTest.rb b/test/excludes/SchemaDumperTest.rb index c18f3521..7cf309b3 100644 --- a/test/excludes/SchemaDumperTest.rb +++ b/test/excludes/SchemaDumperTest.rb @@ -9,4 +9,3 @@ exclude :test_schema_dump_with_correct_timestamp_types_via_add_column_before_rails_7_with_timestamptz_setting, "Re-implementing ourselves because we need CockroachDB specific methods." exclude :test_schema_dump_when_changing_datetime_type_for_an_existing_app, "Re-implementing ourselves because we need CockroachDB specific methods." exclude :test_schema_dumps_check_constraints, "Re-implementing because some constraints are now written in parenthesis" -exclude :test_schema_dumps_unique_constraints, "Re-implementing because DEFERRABLE is not supported by CockroachDB"