Merge branch 'issue-277354' into 'master'

Deduplicate epic iids and add uniqueness constraint

See merge request gitlab-org/gitlab!47081

changelogs/unreleased/issue-277354.yml

+---
+title: Fix duplicate epic iids and add uniqueness constraint
+merge_request: 47081
+author:
+type: fixed

db/post_migrate/20201106134950_deduplicate_epic_iids.rb

+# frozen_string_literal: true
+
+class DeduplicateEpicIids < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  INDEX_NAME = 'index_epics_on_group_id_and_iid'
+
+  disable_ddl_transaction!
+
+  class Epic < ActiveRecord::Base
+  end
+
+  class InternalId < ActiveRecord::Base
+    class << self
+      def generate_next(subject, scope, usage, init)
+        InternalIdGenerator.new(subject, scope, usage, init).generate
+      end
+    end
+
+    # Increments #last_value and saves the record
+    #
+    # The operation locks the record and gathers a `ROW SHARE` lock (in PostgreSQL).
+    # As such, the increment is atomic and safe to be called concurrently.
+    def increment_and_save!
+      update_and_save { self.last_value = (last_value || 0) + 1 }
+    end
+
+    private
+
+    def update_and_save(&block)
+      lock!
+      yield
+      save!
+      last_value
+    end
+  end
+
+  # See app/models/internal_id
+  class InternalIdGenerator
+    attr_reader :subject, :scope, :scope_attrs, :usage, :init
+
+    def initialize(subject, scope, usage, init = nil)
+      @subject = subject
+      @scope = scope
+      @usage = usage
+      @init = init
+
+      raise ArgumentError, 'Scope is not well-defined, need at least one column for scope (given: 0)' if scope.empty? || usage.to_s != 'epics'
+    end
+
+    # Generates next internal id and returns it
+    # init: Block that gets called to initialize InternalId record if not present
+    #       Make sure to not throw exceptions in the absence of records (if this is expected).
+    def generate
+      subject.transaction do
+        # Create a record in internal_ids if one does not yet exist
+        # and increment its last value
+        #
+        # Note this will acquire a ROW SHARE lock on the InternalId record
+        record.increment_and_save!
+      end
+    end
+
+    def record
+      @record ||= (lookup || create_record)
+    end
+
+    def lookup
+      InternalId.find_by(**scope, usage: usage_value)
+    end
+
+    def usage_value
+      4 # see Enums::InternalId - this is the value for epics
+    end
+
+    # Create InternalId record for (scope, usage) combination, if it doesn't exist
+    #
+    # We blindly insert without synchronization. If another process
+    # was faster in doing this, we'll realize once we hit the unique key constraint
+    # violation. We can safely roll-back the nested transaction and perform
+    # a lookup instead to retrieve the record.
+    def create_record
+      raise ArgumentError, 'Cannot initialize without init!' unless init
+
+      instance = subject.is_a?(::Class) ? nil : subject
+
+      subject.transaction(requires_new: true) do
+        InternalId.create!(
+          **scope,
+          usage: usage_value,
+          last_value: init.call(instance, scope) || 0
+        )
+      end
+    rescue ActiveRecord::RecordNotUnique
+      lookup
+    end
+  end
+
+  def up
+    duplicate_epic_ids = ApplicationRecord.connection.execute('SELECT iid, group_id, COUNT(*) FROM epics GROUP BY iid, group_id HAVING COUNT(*) > 1;')
+
+    duplicate_epic_ids.each do |dup|
+      Epic.where(iid: dup['iid'], group_id: dup['group_id']).last(dup['count'] - 1).each do |epic|
+        new_iid = InternalId.generate_next(epic,
+          { namespace_id: epic.group_id },
+          :epics, ->(instance, _) { instance.class.where(group_id: epic.group_id).maximum(:iid) }
+        )
+
+        epic.update!(iid: new_iid)
+      end
+    end
+
+    add_concurrent_index :epics, [:group_id, :iid], unique: true, name: INDEX_NAME
+  end
+
+  def down
+    # only remove the index, as we do not want to create the duplicates back
+    remove_concurrent_index :epics, [:group_id, :iid], name: INDEX_NAME
+  end
+end

db/schema_migrations/20201106134950

+f6e4e62dbd992fc8283f3d7872bb33f1b6bea1b366806caf8f7a65140584c0c1
\ No newline at end of file

db/structure.sql

@@ -20741,6 +20741,8 @@ CREATE INDEX index_epics_on_group_id ON epics USING btree (group_id);
 
 CREATE UNIQUE INDEX index_epics_on_group_id_and_external_key ON epics USING btree (group_id, external_key) WHERE (external_key IS NOT NULL);
 
+CREATE UNIQUE INDEX index_epics_on_group_id_and_iid ON epics USING btree (group_id, iid);
+
 CREATE INDEX index_epics_on_group_id_and_iid_varchar_pattern ON epics USING btree (group_id, ((iid)::character varying) varchar_pattern_ops);
 
 CREATE INDEX index_epics_on_iid ON epics USING btree (iid);

ee/spec/fixtures/lib/gitlab/import_export/group_exports/light/group.json

@@ -297,7 +297,7 @@
       "group_id": 4351,
       "author_id": 1,
       "assignee_id": null,
-      "iid": 1,
+      "iid": 2,
       "updated_by_id": null,
       "last_edited_by_id": null,
       "lock_version": 0,

ee/spec/fixtures/lib/gitlab/import_export/group_exports/light/tree/groups/761/epics.ndjson

 {"id":13622,"milestone_id":null,"group_id":4351,"author_id":1,"assignee_id":null,"iid":1,"updated_by_id":null,"last_edited_by_id":null,"lock_version":0,"start_date":null,"end_date":null,"last_edited_at":null,"created_at":"2019-11-20T17:02:09.754Z","updated_at":"2019-11-20T18:38:40.054Z","title":"Provident neque consequatur numquam ad laboriosam voluptatem magnam.","description":"Fugit nisi est ut numquam quia rerum vitae qui. Et in est aliquid voluptas et ut vitae. In distinctio voluptates ut deleniti iste.\n\nReiciendis eum sunt vero blanditiis at quia. Voluptate eum facilis illum ea distinctio maiores. Doloribus aut nemo ea distinctio.\n\nNihil cum distinctio voluptates quam. Laboriosam distinctio ea accusantium soluta perspiciatis nesciunt impedit. Id qui natus quis minima voluptatum velit ut reprehenderit. Molestiae quia est harum sapiente rem error architecto id. Et minus ipsa et ut ut.","start_date_sourcing_milestone_id":null,"due_date_sourcing_milestone_id":null,"start_date_fixed":null,"due_date_fixed":null,"start_date_is_fixed":null,"due_date_is_fixed":null,"closed_by_id":null,"closed_at":null,"parent_id":null,"relative_position":null,"state":"opened","start_date_sourcing_epic_id":null,"due_date_sourcing_epic_id":null,"notes":[{"id":44164,"note":"added epic &2 as child epic","noteable_type":"Epic","author_id":1,"created_at":"2019-11-20T18:38:26.689Z","updated_at":"2019-11-20T18:38:26.724Z","project_id":null,"attachment":{"url":null},"line_code":null,"commit_id":null,"noteable_id":13622,"system":true,"st_diff":null,"updated_by_id":null,"position":null,"original_position":null,"resolved_at":null,"resolved_by_id":null,"discussion_id":"133f0c3001860fa8d2031e398a65db74477378c4","change_position":null,"resolved_by_push":null,"review_id":null,"type":null,"author":{"name":"Administrator"},"award_emoji":[{"id":12,"name":"drum","user_id":1,"awardable_type":"Note","awardable_id":44170,"created_at":"2019-11-05T15:32:21.287Z","updated_at":"2019-11-05T15:32:21.287Z"}]}],"award_emoji":[{"id":12,"name":"thumbsup","user_id":1,"awardable_type":"Epic","awardable_id":13622,"created_at":"2019-11-05T15:37:21.287Z","updated_at":"2019-11-05T15:37:21.287Z"}]}
-{"id":13623,"milestone_id":null,"group_id":4351,"author_id":1,"assignee_id":null,"iid":1,"updated_by_id":null,"last_edited_by_id":null,"lock_version":0,"start_date":null,"end_date":null,"last_edited_at":null,"created_at":"2019-12-20T17:02:09.754Z","updated_at":"2019-12-20T18:38:40.054Z","title":"Provident neque consequatur numquam ad voluptatem magnam.","description":"Fugit nisi","start_date_sourcing_milestone_id":null,"due_date_sourcing_milestone_id":null,"start_date_fixed":null,"due_date_fixed":null,"start_date_is_fixed":null,"due_date_is_fixed":null,"closed_by_id":null,"closed_at":null,"parent_id":null,"relative_position":null,"state":"closed","start_date_sourcing_epic_id":null,"due_date_sourcing_epic_id":null,"notes":[]}
+{"id":13623,"milestone_id":null,"group_id":4351,"author_id":1,"assignee_id":null,"iid":2,"updated_by_id":null,"last_edited_by_id":null,"lock_version":0,"start_date":null,"end_date":null,"last_edited_at":null,"created_at":"2019-12-20T17:02:09.754Z","updated_at":"2019-12-20T18:38:40.054Z","title":"Provident neque consequatur numquam ad voluptatem magnam.","description":"Fugit nisi","start_date_sourcing_milestone_id":null,"due_date_sourcing_milestone_id":null,"start_date_fixed":null,"due_date_fixed":null,"start_date_is_fixed":null,"due_date_is_fixed":null,"closed_by_id":null,"closed_at":null,"parent_id":null,"relative_position":null,"state":"closed","start_date_sourcing_epic_id":null,"due_date_sourcing_epic_id":null,"notes":[]}

spec/migrations/deduplicate_epic_iids_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20201106134950_deduplicate_epic_iids.rb')
+
+RSpec.describe DeduplicateEpicIids, :migration, schema: 20201106082723 do
+  let(:routes) { table(:routes) }
+  let(:epics) { table(:epics) }
+  let(:users) { table(:users) }
+  let(:namespaces) { table(:namespaces) }
+
+  let!(:group) { create_group('foo') }
+  let!(:user) { users.create!(email: 'test@example.com', projects_limit: 100, username: 'test') }
+  let!(:dup_epic1) { epics.create!(iid: 1, title: 'epic 1', group_id: group.id, author_id: user.id, created_at: Time.now, updated_at: Time.now, title_html: 'any') }
+  let!(:dup_epic2) { epics.create!(iid: 1, title: 'epic 2', group_id: group.id, author_id: user.id, created_at: Time.now, updated_at: Time.now, title_html: 'any') }
+  let!(:dup_epic3) { epics.create!(iid: 1, title: 'epic 3', group_id: group.id, author_id: user.id, created_at: Time.now, updated_at: Time.now, title_html: 'any') }
+
+  it 'deduplicates epic iids', :aggregate_failures do
+    duplicate_epics_count = epics.where(iid: 1, group_id: group.id).count
+    expect(duplicate_epics_count).to eq 3
+
+    migrate!
+
+    duplicate_epics_count = epics.where(iid: 1, group_id: group.id).count
+    expect(duplicate_epics_count).to eq 1
+    expect(dup_epic1.reload.iid).to eq 1
+    expect(dup_epic2.reload.iid).to eq 2
+    expect(dup_epic3.reload.iid).to eq 3
+  end
+
+  def create_group(path)
+    namespaces.create!(name: path, path: path, type: 'Group').tap do |namespace|
+      routes.create!(path: namespace.path, name: namespace.name, source_id: namespace.id, source_type: 'Namespace')
+    end
+  end
+end

spec/migrations/migrate_discussion_id_on_promoted_epics_spec.rb

@@ -25,7 +25,7 @@ RSpec.describe MigrateDiscussionIdOnPromotedEpics do
   end
 
   def create_epic
-    epics.create!(author_id: user.id, iid: 1,
+    epics.create!(author_id: user.id, iid: epics.maximum(:iid).to_i + 1,
                   group_id: namespace.id,
                   title: 'Epic with discussion',
                   title_html: 'Epic with discussion')