Migrations for adding issue_id to versions table

These migrations do the following:

- Adds a new `issue_id` column to `versions`. This fixes an n+1 problem
  when loading versions for an issue in GraphQL as AR can now load from
  cache
- Change the unique restraint on versions.sha to be scoped to `issue_id`
  as in order to import version data, we need to allow duplicate `sha`
  values for versions
- Update all versions with an `issue_id`

https://gitlab.com/gitlab-org/gitlab-ee/issues/11090

db/migrate/20190715042813_add_issue_id_to_versions.rb

+# frozen_string_literal: true
+
+class AddIssueIdToVersions < ActiveRecord::Migration[5.2]
+  DOWNTIME = false
+
+  def up
+    add_reference :design_management_versions, :issue, index: true, foreign_key: { on_delete: :cascade }
+  end
+
+  def down
+    remove_reference :design_management_versions, :issue
+  end
+end

db/migrate/20190715043954_set_issue_id_for_all_versions.rb

+# frozen_string_literal: true
+
+class SetIssueIdForAllVersions < ActiveRecord::Migration[5.2]
+  DOWNTIME = false
+
+  def up
+    execute('UPDATE design_management_versions as versions SET issue_id = (
+      SELECT design_management_designs.issue_id
+        FROM design_management_designs
+        INNER JOIN design_management_designs_versions ON design_management_designs.id = design_management_designs_versions.design_id
+        WHERE design_management_designs_versions.version_id = versions.id
+        LIMIT 1
+    )')
+  end
+
+  def down
+    # no-op
+  end
+end

db/post_migrate/20190715043944_remove_sha_index_from_versions.rb

+# frozen_string_literal: true
+
+class RemoveShaIndexFromVersions < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    remove_concurrent_index :design_management_versions, :sha
+  end
+
+  def down
+    add_concurrent_index :design_management_versions, :sha, unique: true, using: :btree
+  end
+end

db/post_migrate/20190715044501_add_unique_issue_id_sha_index_to_versions.rb

+# frozen_string_literal: true
+
+class AddUniqueIssueIdShaIndexToVersions < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :design_management_versions, [:sha, :issue_id], unique: true, using: :btree
+  end
+
+  def down
+    remove_concurrent_index :design_management_versions, [:sha, :issue_id]
+  end
+end

db/schema.rb

@@ -1116,7 +1116,9 @@ ActiveRecord::Schema.define(version: 2019_07_25_012225) do
 
   create_table "design_management_versions", force: :cascade do |t|
     t.binary "sha", null: false
-    t.index ["sha"], name: "index_design_management_versions_on_sha", unique: true
+    t.bigint "issue_id"
+    t.index ["issue_id"], name: "index_design_management_versions_on_issue_id"
+    t.index ["sha", "issue_id"], name: "index_design_management_versions_on_sha_and_issue_id", unique: true
   end
 
   create_table "draft_notes", force: :cascade do |t|
@@ -3699,8 +3701,9 @@ ActiveRecord::Schema.define(version: 2019_07_25_012225) do
   add_foreign_key "deployments", "projects", name: "fk_b9a3851b82", on_delete: :cascade
   add_foreign_key "design_management_designs", "issues", on_delete: :cascade
   add_foreign_key "design_management_designs", "projects", on_delete: :cascade
-  add_foreign_key "design_management_designs_versions", "design_management_designs", column: "design_id", on_delete: :cascade
-  add_foreign_key "design_management_designs_versions", "design_management_versions", column: "version_id", on_delete: :cascade
+  add_foreign_key "design_management_designs_versions", "design_management_designs", column: "design_id", name: "fk_03c671965c", on_delete: :cascade
+  add_foreign_key "design_management_designs_versions", "design_management_versions", column: "version_id", name: "fk_f4d25ba00c", on_delete: :cascade
+  add_foreign_key "design_management_versions", "issues", on_delete: :cascade
   add_foreign_key "draft_notes", "merge_requests", on_delete: :cascade
   add_foreign_key "draft_notes", "users", column: "author_id", on_delete: :cascade
   add_foreign_key "elasticsearch_indexed_namespaces", "namespaces", on_delete: :cascade

ee/app/models/design_management/version.rb

@@ -4,6 +4,7 @@ module DesignManagement
   class Version < ApplicationRecord
     include ShaAttribute
 
+    belongs_to :issue
     has_many :design_versions
     has_many :designs,
              through: :design_versions,
@@ -12,7 +13,7 @@ module DesignManagement
              inverse_of: :versions
 
     validates :sha, presence: true
-    validates :sha, uniqueness: { case_sensitive: false }
+    validates :sha, uniqueness: { case_sensitive: false, scope: :issue_id }
 
     sha_attribute :sha
 
@@ -23,7 +24,9 @@ module DesignManagement
     scope :ordered, -> { order(id: :desc) }
 
     def self.create_for_designs(designs, sha)
-      version = safe_find_or_create_by!(sha: sha)
+      issue_id = designs.first.issue_id
+
+      version = safe_find_or_create_by!(sha: sha, issue_id: issue_id)
 
       rows = designs.map do |design|
         { design_id: design.id, version_id: version.id }
@@ -33,9 +36,5 @@ module DesignManagement
 
       version
     end
-
-    def issue
-      designs.take.issue
-    end
   end
 end

ee/spec/factories/design_management/versions.rb

@@ -3,5 +3,6 @@
 FactoryBot.define do
   factory :design_version, class: DesignManagement::Version do
     sequence(:sha) { |n| Digest::SHA1.hexdigest("commit-like-#{n}") }
+    issue { designs.first&.issue || create(:issue) }
   end
 end

ee/spec/models/design_management/version_spec.rb

@@ -29,7 +29,7 @@ describe DesignManagement::Version do
 
     it { is_expected.to be_valid }
     it { is_expected.to validate_presence_of(:sha) }
-    it { is_expected.to validate_uniqueness_of(:sha).case_insensitive }
+    it { is_expected.to validate_uniqueness_of(:sha).scoped_to(:issue_id).case_insensitive }
   end
 
   describe "scopes" do
@@ -70,13 +70,4 @@ describe DesignManagement::Version do
       expect(version.designs).to contain_exactly(*designs)
     end
   end
-
-  describe "#issue" do
-    it "gets the issue for the linked design" do
-      version = create(:design_version)
-      design = create(:design, versions: [version])
-
-      expect(version.issue).to eq(design.issue)
-    end
-  end
 end

spec/migrations/set_issue_id_for_all_versions_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'migrate', '20190715043954_set_issue_id_for_all_versions.rb')
+
+describe SetIssueIdForAllVersions, :migration do
+  let(:projects) { table(:projects) }
+  let(:issues) { table(:issues) }
+  let(:designs) { table(:design_management_designs) }
+  let(:designs_versions) { table(:design_management_designs_versions) }
+  let(:versions) { table(:design_management_versions) }
+
+  before do
+    @project = projects.create!(name: 'gitlab', path: 'gitlab-org/gitlab-ce', namespace_id: 1)
+
+    @issue_1 = issues.create!(description: 'first', project_id: @project.id)
+    @issue_2 = issues.create!(description: 'second', project_id: @project.id)
+
+    @design_1 = designs.create!(issue_id: @issue_1.id, filename: 'homepage-1.jpg', project_id: @project.id)
+    @design_2 = designs.create!(issue_id: @issue_2.id, filename: 'homepage-2.jpg', project_id: @project.id)
+
+    @version_1 = versions.create!(sha: 'foo')
+    @version_2 = versions.create!(sha: 'bar')
+
+    designs_versions.create!(version_id: @version_1.id, design_id: @design_1.id)
+    designs_versions.create!(version_id: @version_2.id, design_id: @design_2.id)
+  end
+
+  it 'correctly sets issue_id' do
+    expect(versions.where(issue_id: nil).count).to eq(2)
+
+    migrate!
+
+    expect(versions.where(issue_id: nil).count).to eq(0)
+    expect(versions.find(@version_1.id).issue_id).to eq(@issue_1.id)
+    expect(versions.find(@version_2.id).issue_id).to eq(@issue_2.id)
+  end
+end