add if_not_exists to add_foreign_key

also fix delay_validation with rails 5.2, and constraints can be re-validated
without dropping them

Change-Id: Iab4d2dc374c7b4707d67323d05e8e8e8ff203e47
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/239656
Tested-by: Service Cloud Jenkins <[email protected]>
Reviewed-by: Ethan Vizitei <[email protected]>
QA-Review: Ethan Vizitei <[email protected]>
Product-Review: Ethan Vizitei <[email protected]>

config/initializers/active_record.rb

@@ -1400,30 +1400,6 @@ def execute_migration_in_transaction(migration, direct)
 ActiveRecord::Tasks::DatabaseTasks.migrations_paths = ActiveRecord::Migrator.migrations_paths
 
 ActiveRecord::ConnectionAdapters::SchemaStatements.class_eval do
-  # in anticipation of having to re-run migrations due to integrity violations or
-  # killing stuff that is holding locks too long
-  def add_foreign_key_if_not_exists(from_table, to_table, options = {})
-    options[:column] ||= "#{to_table.to_s.singularize}_id"
-    column = options[:column]
-    case self.adapter_name
-    when 'PostgreSQL'
-      foreign_key_name = foreign_key_name(from_table, options)
-      schema = @config[:use_qualified_names] ? quote(shard.name) : 'current_schema()'
-      value = select_value("SELECT convalidated FROM pg_constraint INNER JOIN pg_namespace ON pg_namespace.oid=connamespace WHERE conname='#{foreign_key_name}' AND nspname=#{schema}")
-      if value == 'f'
-        execute("ALTER TABLE #{quote_table_name(from_table)} DROP CONSTRAINT #{quote_table_name(foreign_key_name)}")
-      elsif value
-        return
-      end
-
-      add_foreign_key(from_table, to_table, options)
-    else
-      foreign_key_name = foreign_key_name(from_table, column, options)
-      return if foreign_keys(from_table).find { |k| k.options[:name] == foreign_key_name }
-      add_foreign_key(from_table, to_table, options)
-    end
-  end
-
   def find_foreign_key(from_table, to_table, column: nil)
     column ||= "#{to_table.to_s.singularize}_id"
     foreign_keys(from_table).find do |key|

config/initializers/postgresql_adapter.rb

@@ -58,28 +58,33 @@ def quote_text(value)
     end
   end
 
-  def add_foreign_key(from_table, to_table, options = {})
-    raise ArgumentError, "Cannot specify custom options with :delay_validation" if options[:options] && options[:delay_validation]
+  def add_foreign_key(from_table, to_table, delay_validation: false, if_not_exists: false, **options)
+    raise ArgumentError, "Cannot specify custom options with :delay_validation" if options[:options] && delay_validation
 
     # pointless if we're in a transaction
-    options.delete(:delay_validation) if open_transactions > 0
+    delay_validation = false if open_transactions > 0
     options[:column] ||= "#{to_table.to_s.singularize}_id"
     column = options[:column]
 
     foreign_key_name = foreign_key_name(from_table, options)
 
-    if options[:delay_validation]
-      options[:options] = 'NOT VALID'
+    if if_not_exists || delay_validation
+      schema = @config[:use_qualified_names] ? quote(shard.name) : 'current_schema()'
+      valid = select_value("SELECT convalidated FROM pg_constraint INNER JOIN pg_namespace ON pg_namespace.oid=connamespace WHERE conname='#{foreign_key_name}' AND nspname=#{schema}", "SCHEMA")
+      return if valid == true && if_not_exists
+    end
+
+    if delay_validation
+      options[:validate] = false
       # NOT VALID doesn't fully work through 9.3 at least, so prime the cache to make
       # it as fast as possible. Note that a NOT EXISTS would be faster, but this is
       # the query postgres does for the VALIDATE CONSTRAINT, so we want exactly this
       # query to be warm
       execute("SELECT fk.#{column} FROM #{quote_table_name(from_table)} fk LEFT OUTER JOIN #{quote_table_name(to_table)} pk ON fk.#{column}=pk.id WHERE pk.id IS NULL AND fk.#{column} IS NOT NULL LIMIT 1")
     end
 
-    super(from_table, to_table, options)
-
-    execute("ALTER TABLE #{quote_table_name(from_table)} VALIDATE CONSTRAINT #{quote_column_name(foreign_key_name)}") if options[:delay_validation]
+    super(from_table, to_table, **options) unless valid == false
+    validate_constraint(from_table, foreign_key_name) if delay_validation
   end
 
   def set_standard_conforming_strings

db/migrate/20190402162707_remove_course_copy_foreign_keys.rb

@@ -9,9 +9,9 @@ def up
   end
 
   def down
-    add_foreign_key_if_not_exists :content_migrations, :courses, :column => :source_course_id, :delay_validation => true
-    add_foreign_key_if_not_exists :content_migrations, :attachments, :delay_validation => true
-    add_foreign_key_if_not_exists :content_exports, :content_migrations, :delay_validation => true
-    add_foreign_key_if_not_exists :folders, :cloned_items, :delay_validation => true
+    add_foreign_key :content_migrations, :courses, :column => :source_course_id, :delay_validation => true, if_not_exists: true
+    add_foreign_key :content_migrations, :attachments, :delay_validation => true, if_not_exists: true
+    add_foreign_key :content_exports, :content_migrations, :delay_validation => true, if_not_exists: true
+    add_foreign_key :folders, :cloned_items, :delay_validation => true, if_not_exists: true
   end
 end

db/migrate/20190904204849_remove_correct_content_migration_foreign_key.rb

@@ -22,6 +22,6 @@ def up
   end
 
   def down
-    add_foreign_key_if_not_exists :content_migrations, :attachments, :column => :attachment_id, :delay_validation => true
+    add_foreign_key :content_migrations, :attachments, :column => :attachment_id, :delay_validation => true, if_not_exists: true
   end
 end

db/migrate/20200309222710_add_root_account_id_to_lti_line_items.rb

@@ -20,6 +20,6 @@ class AddRootAccountIdToLtiLineItems < ActiveRecord::Migration[5.2]
 
   def change
     add_column :lti_line_items, :root_account_id, :integer, limit: 8, if_not_exists: true
-    add_foreign_key :lti_line_items, :accounts, column: :root_account_id
+    add_foreign_key :lti_line_items, :accounts, column: :root_account_id, if_not_exists: true
   end
 end

db/migrate/20200410155410_add_root_account_id_to_content_participations.rb

@@ -21,6 +21,6 @@ class AddRootAccountIdToContentParticipations < ActiveRecord::Migration[5.2]
 
   def change
     add_column :content_participations, :root_account_id, :integer, limit: 8, if_not_exists: true
-    add_foreign_key :content_participations, :accounts, column: :root_account_id
+    add_foreign_key :content_participations, :accounts, column: :root_account_id, if_not_exists: true
   end
 end

db/migrate/20200410160448_add_root_account_id_to_content_participation_counts.rb

@@ -21,6 +21,6 @@ class AddRootAccountIdToContentParticipationCounts < ActiveRecord::Migration[5.2
 
   def change
     add_column :content_participation_counts, :root_account_id, :integer, limit: 8, if_not_exists: true
-    add_foreign_key :content_participation_counts, :accounts, column: :root_account_id
+    add_foreign_key :content_participation_counts, :accounts, column: :root_account_id, if_not_exists: true
   end
 end

spec/initializers/active_record_spec.rb

@@ -362,6 +362,20 @@ module ActiveRecord
       it "allows if_not_exists on add_column" do
         expect { User.connection.add_column(:enrollments, :user_id, :bigint, if_not_exists: true) }.not_to raise_exception
       end
+
+      it "allows if_not_exists on add_foreign_key" do
+        expect { User.connection.add_foreign_key(:enrollments, :users, if_not_exists: true) }.not_to raise_exception
+      end
+
+      it "add_foreign_key automatically validates an invalid constraint with delay_validation" do
+        expect do
+          User.connection.remove_foreign_key(:enrollments, column: :user_id)
+          User.connection.add_foreign_key(:enrollments, :users, validate: false)
+          # so that delay_validation doesn't get ignored
+          allow(User.connection).to receive(:open_transactions).and_return(0)
+          User.connection.add_foreign_key(:enrollments, :users, delay_validation: true)
+        end.not_to raise_exception
+      end
     end
   end
 end