Merge branch 'reschedule_blocked_by' into 'master'

Re-enqueue blocked_by link replacement

See merge request gitlab-org/gitlab!46770

app/models/issue_link.rb

@@ -10,6 +10,7 @@ class IssueLink < ApplicationRecord
   validates :target, presence: true
   validates :source, uniqueness: { scope: :target_id, message: 'is already related' }
   validate :check_self_relation
+  validate :check_opposite_relation
 
   scope :for_source_issue, ->(issue) { where(source_id: issue.id) }
   scope :for_target_issue, ->(issue) { where(target_id: issue.id) }
@@ -33,6 +34,14 @@ class IssueLink < ApplicationRecord
       errors.add(:source, 'cannot be related to itself')
     end
   end
+
+  def check_opposite_relation
+    return unless source && target
+
+    if IssueLink.find_by(source: target, target: source)
+      errors.add(:source, 'is already related to this issue')
+    end
+  end
 end
 
 IssueLink.prepend_if_ee('EE::IssueLink')

changelogs/unreleased/reschedule_blocked_by.yml

+---
+title: Reschedule again background migration which convers 'blocked_by' issue links
+  to 'block'
+merge_request: 46770
+author:
+type: changed

db/post_migrate/20201015073808_schedule_blocked_by_links_replacement.rb

@@ -19,10 +19,8 @@ class ScheduleBlockedByLinksReplacement < ActiveRecord::Migration[6.0]
   end
 
   def up
-    relation = IssueLink.where(link_type: 2)
-
-    queue_background_migration_jobs_by_range_at_intervals(
-      relation, MIGRATION, INTERVAL, batch_size: BATCH_SIZE)
+    # no-op
+    # superseded by db/post_migrate/20201102073808_schedule_blocked_by_links_replacement_second_try.rb
   end
 
   def down

db/post_migrate/20201102073808_schedule_blocked_by_links_replacement_second_try.rb

+# frozen_string_literal: true
+
+class ScheduleBlockedByLinksReplacementSecondTry < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  INTERVAL = 2.minutes
+  # at the time of writing there were 12931 blocked_by issues:
+  # estimated time is 13 batches * 2 minutes -> 26 minutes
+  BATCH_SIZE = 1000
+  MIGRATION = 'ReplaceBlockedByLinks'
+
+  disable_ddl_transaction!
+
+  class IssueLink < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'issue_links'
+  end
+
+  def up
+    relation = IssueLink.where(link_type: 2)
+
+    queue_background_migration_jobs_by_range_at_intervals(
+      relation, MIGRATION, INTERVAL, batch_size: BATCH_SIZE)
+  end
+
+  def down
+  end
+end

db/schema_migrations/20201102073808

+153b437ac481f4d79cd5bdee45dd3932f3cb58bf5dce793573c4b651a3b9314f
\ No newline at end of file

lib/gitlab/background_migration/replace_blocked_by_links.rb

@@ -12,14 +12,19 @@ module Gitlab
         blocked_by_links = IssueLink.where(id: start_id..stop_id).where(link_type: 2)
 
         ActiveRecord::Base.transaction do
-          # if there is duplicit bi-directional relation (issue2 is blocked by issue1
-          # and issue1 already links issue2), then we can just delete 'blocked by'.
-          # This should be rare as we have a pre-create check which checks if issues are
-          # already linked
-          blocked_by_links
+          # There could be two edge cases:
+          # 1) issue1 is blocked by issue2 AND issue2 blocks issue1 (type 1)
+          # 2) issue1 is blocked by issue2 AND issue2 is related to issue1 (type 0)
+          # In both cases cases we couldn't convert blocked by relation to
+          # `issue2 blocks issue` because there is already a link with the same
+          # source/target id. To avoid these conflicts, we first delete any
+          # "opposite" links before we update `blocked by` relation.  This
+          # should be rare as we have a pre-create check which checks if issues
+          # are already linked
+          opposite_ids = blocked_by_links
+            .select('opposite_links.id')
             .joins('INNER JOIN issue_links as opposite_links ON issue_links.source_id = opposite_links.target_id AND issue_links.target_id = opposite_links.source_id')
-            .where('opposite_links.link_type': 1)
-            .delete_all
+          IssueLink.where(id: opposite_ids).delete_all
 
           blocked_by_links.update_all('source_id=target_id,target_id=source_id,link_type=1')
         end

spec/lib/gitlab/background_migration/replace_blocked_by_links_spec.rb

@@ -9,28 +9,34 @@ RSpec.describe Gitlab::BackgroundMigration::ReplaceBlockedByLinks, schema: 20201
   let(:issue2) { table(:issues).create!(project_id: project.id, title: 'b') }
   let(:issue3) { table(:issues).create!(project_id: project.id, title: 'c') }
   let(:issue_links) { table(:issue_links) }
-  let!(:blocks_link) { issue_links.create!(source_id: issue1.id, target_id: issue2.id, link_type: 1) }
-  let!(:bidirectional_link) { issue_links.create!(source_id: issue2.id, target_id: issue1.id, link_type: 2) }
-  let!(:blocked_link) { issue_links.create!(source_id: issue1.id, target_id: issue3.id, link_type: 2) }
+  let!(:blocked_link1) { issue_links.create!(source_id: issue2.id, target_id: issue1.id, link_type: 2) }
+  let!(:opposite_link1) { issue_links.create!(source_id: issue1.id, target_id: issue2.id, link_type: 1) }
+  let!(:blocked_link2) { issue_links.create!(source_id: issue1.id, target_id: issue3.id, link_type: 2) }
+  let!(:opposite_link2) { issue_links.create!(source_id: issue3.id, target_id: issue1.id, link_type: 0) }
+  let!(:nochange_link) { issue_links.create!(source_id: issue2.id, target_id: issue3.id, link_type: 1) }
 
   subject { described_class.new.perform(issue_links.minimum(:id), issue_links.maximum(:id)) }
 
-  it 'deletes issue links where opposite relation already exists' do
-    expect { subject }.to change { issue_links.count }.by(-1)
+  it 'deletes any opposite relations' do
+    subject
+
+    expect(issue_links.ids).to match_array([nochange_link.id, blocked_link1.id, blocked_link2.id])
   end
 
   it 'ignores issue links other than blocked_by' do
     subject
 
-    expect(blocks_link.reload.link_type).to eq(1)
+    expect(nochange_link.reload.link_type).to eq(1)
   end
 
   it 'updates blocked_by issue links' do
     subject
 
-    link = blocked_link.reload
-    expect(link.link_type).to eq(1)
-    expect(link.source_id).to eq(issue3.id)
-    expect(link.target_id).to eq(issue1.id)
+    expect(blocked_link1.reload.link_type).to eq(1)
+    expect(blocked_link1.source_id).to eq(issue1.id)
+    expect(blocked_link1.target_id).to eq(issue2.id)
+    expect(blocked_link2.reload.link_type).to eq(1)
+    expect(blocked_link2.source_id).to eq(issue3.id)
+    expect(blocked_link2.target_id).to eq(issue1.id)
   end
 end

spec/migrations/schedule_blocked_by_links_replacement_spec.rb → spec/migrations/schedule_blocked_by_links_replacement_second_try_spec.rb

 # frozen_string_literal: true
 
 require 'spec_helper'
-require Rails.root.join('db', 'post_migrate', '20201015073808_schedule_blocked_by_links_replacement')
+require Rails.root.join('db', 'post_migrate', '20201102073808_schedule_blocked_by_links_replacement_second_try')
 
-RSpec.describe ScheduleBlockedByLinksReplacement do
+RSpec.describe ScheduleBlockedByLinksReplacementSecondTry do
   let(:namespace) { table(:namespaces).create!(name: 'gitlab', path: 'gitlab-org') }
   let(:project) { table(:projects).create!(namespace_id: namespace.id, name: 'gitlab') }
   let(:issue1) { table(:issues).create!(project_id: project.id, title: 'a') }

spec/models/issue_link_spec.rb

@@ -27,7 +27,14 @@ RSpec.describe IssueLink do
                        .with_message(/already related/)
     end
 
-    context 'self relation' do
+    it 'is not valid if an opposite link already exists' do
+      issue_link = build(:issue_link, source: subject.target, target: subject.source)
+
+      expect(issue_link).to be_invalid
+      expect(issue_link.errors[:source]).to include('is already related to this issue')
+    end
+
+    context 'when it relates to itself' do
       let(:issue) { create :issue }
 
       context 'cannot be validated' do