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

Author: BuonOmo

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
 

test/cases/migration/unique_constraint_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

test/cases/schema_dumper_test.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()"

test/excludes/ActiveRecord/ConnectionAdapters/PostgreSQLAdapterTest.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"