Fix unstable temporary check constraint in case of an error

When changing a column null value from true to false (namely, the column
becomes NOT NULL) and `safe mode` is enabled:
-  we add a NOT VALID check constraint
- then commit the transaction
-  and finally validate the check constraint in another migration.

The issue arises when the validation of check constraint fails. This
leaves an invalid check constraint in the database and it is NOT
dropped in a cleanup phase.
After making sure that all values are NOT NULL), we then retry to run
`rails db:migrate`. However this fails, because, since the check
constraint already exists, it will result in an error raised stating
that said check constraint **already** exists.

This commit fixes the issue in both ways:
- Firstly, if we're already in this unstable state of a NOT VALID check
  constraint already existing in the database, we avoid the recreation
  of the constraint and simply ignore this step
- Secondly, in case the check validation fails, we rescue the exception
  and execute the drop of the check constraint to avoid leaving the
  database in an unstable state.

lib/strong_migrations/adapters/postgresql_adapter.rb

@@ -127,16 +127,15 @@ def change_type_safe?(table, column, type, options, existing_column, existing_ty
         safe
       end
 
-      def constraints(table_name)
+      def constraints(table_name, include_invalid: false)
         query = <<~SQL
           SELECT
             conname AS name,
             pg_get_constraintdef(oid) AS def
           FROM
             pg_constraint
           WHERE
-            contype = 'c' AND
-            convalidated AND
+            contype = 'c' #{ "AND convalidated" if !include_invalid } AND
             conrelid = #{connection.quote(connection.quote_table_name(table_name))}::regclass
         SQL
         select_all(query.squish).to_a

lib/strong_migrations/checks.rb

@@ -228,13 +228,15 @@ def check_change_column_null(*args)
       table, column, null, default = args
       if !null
         if postgresql?
-          safe = adapter.constraints(table).any? { |c| c["def"] == "CHECK ((#{column} IS NOT NULL))" || c["def"] == "CHECK ((#{connection.quote_column_name(column)} IS NOT NULL))" }
+          table_constraints = adapter.constraints(table, include_invalid: true)
+          safe = table_constraints.any? { |c| c["def"] == "CHECK ((#{column} IS NOT NULL))" || c["def"] == "CHECK ((#{connection.quote_column_name(column)} IS NOT NULL))" }
 
           unless safe
             # match https://github.com/nullobject/rein
             constraint_name = "#{table}_#{column}_null"
+            invalid_constraint_exist = table_constraints.any? { |c| c['name'] == constraint_name }
 
-            add_code = constraint_str("ALTER TABLE %s ADD CONSTRAINT %s CHECK (%s IS NOT NULL) NOT VALID", [table, constraint_name, column])
+            add_code = constraint_str("ALTER TABLE %s ADD CONSTRAINT %s CHECK (%s IS NOT NULL) NOT VALID", [table, constraint_name, column]) unless invalid_constraint_exist
             validate_code = constraint_str("ALTER TABLE %s VALIDATE CONSTRAINT %s", [table, constraint_name])
             remove_code = constraint_str("ALTER TABLE %s DROP CONSTRAINT %s", [table, constraint_name])
 

lib/strong_migrations/safe_methods.rb

@@ -80,9 +80,18 @@ def safe_change_column_null(add_code, validate_code, change_args, remove_code, d
           end
 
           @migration.safety_assured do
-            @migration.execute(add_code)
-            disable_transaction
-            @migration.execute(validate_code)
+            if add_code
+              @migration.execute(add_code)
+              disable_transaction
+            else
+              self.transaction_disabled = true
+            end
+            begin
+              @migration.execute(validate_code)
+            rescue ActiveRecord::StatementInvalid
+              @migration.execute(remove_code)
+              raise
+            end
           end
           if change_args
             @migration.change_column_null(*change_args)

test/safe_by_default_test.rb

@@ -115,7 +115,31 @@ def test_add_check_constraint_extra_arguments
   def test_change_column_null
     skip unless postgresql?
 
+    User.create!(name: 'Jeff')
+
     assert_safe ChangeColumnNull
+  ensure
+    User.delete_all
+  end
+
+  def test_change_column_null_cleanup_constraint
+    skip unless postgresql?
+
+    # Create a user without a name
+    user = User.create!(name: nil)
+    temporary_constraint_name = 'users_name_null'
+
+    error = assert_raises(ActiveRecord::StatementInvalid) do
+      assert_safe ChangeColumnNull
+    end
+
+    assert_match  "PG::CheckViolation: ERROR:  check constraint \"#{temporary_constraint_name}\" of relation \"users\" is violated by some row\n", error.message
+
+    user.update!(name: 'jeff')
+
+    assert_safe ChangeColumnNull
+  ensure
+    User.delete_all
   end
 
   def test_change_column_null_default