Merge branch '35938-add-unique-constraint' into 'master'

Add a unique constraint to `software_licenses.name` column

See merge request gitlab-org/gitlab!19840

db/post_migrate/20191108202723_add_unique_constraint_to_software_licenses.rb

+# frozen_string_literal: true
+
+class AddUniqueConstraintToSoftwareLicenses < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+  DOWNTIME = false
+  NEW_INDEX = 'index_software_licenses_on_unique_name'
+  OLD_INDEX = 'index_software_licenses_on_name'
+
+  disable_ddl_transaction!
+
+  # 12 software licenses will be removed on GitLab.com
+  # 0 software license policies will be updated on GitLab.com
+  def up(attempts: 100)
+    remove_redundant_software_licenses!
+
+    add_concurrent_index :software_licenses, :name, unique: true, name: NEW_INDEX
+    remove_concurrent_index :software_licenses, :name, name: OLD_INDEX
+  rescue ActiveRecord::RecordNotUnique
+    retry if (attempts -= 1) > 0
+
+    raise StandardError, <<~EOS
+      Failed to add an unique index to software_licenses, despite retrying the
+      migration 100 times.
+
+      See https://gitlab.com/gitlab-org/gitlab/merge_requests/19840.
+    EOS
+  end
+
+  def down
+    remove_concurrent_index :software_licenses, :name, unique: true, name: NEW_INDEX
+    add_concurrent_index :software_licenses, :name, name: OLD_INDEX
+  end
+
+  private
+
+  def remove_redundant_software_licenses!
+    redundant_software_licenses = execute <<~SQL
+      SELECT min(id) id, name
+      FROM software_licenses
+      WHERE name IN (select name from software_licenses group by name having count(name) > 1)
+      GROUP BY name
+    SQL
+    say "Detected #{redundant_software_licenses.count} duplicates."
+
+    redundant_software_licenses.each_row do |id, name|
+      say_with_time("Reassigning policies that reference software license #{name}.") do
+        duplicates = software_licenses.where.not(id: id).where(name: name)
+
+        software_license_policies
+          .where(software_license_id: duplicates)
+          .update_all(software_license_id: id)
+
+        duplicates.delete_all
+      end
+    end
+  end
+
+  def table(name)
+    Class.new(ActiveRecord::Base) { self.table_name = name }
+  end
+
+  def software_licenses
+    @software_licenses ||= table(:software_licenses)
+  end
+
+  def software_license_policies
+    @software_license_policies ||= table(:software_license_policies)
+  end
+end

db/schema.rb

@@ -3698,7 +3698,7 @@ ActiveRecord::Schema.define(version: 2019_12_02_031812) do
   create_table "software_licenses", id: :serial, force: :cascade do |t|
     t.string "name", null: false
     t.string "spdx_identifier", limit: 255
-    t.index ["name"], name: "index_software_licenses_on_name"
+    t.index ["name"], name: "index_software_licenses_on_unique_name", unique: true
     t.index ["spdx_identifier"], name: "index_software_licenses_on_spdx_identifier"
   end
 

ee/app/models/software_license.rb

@@ -5,7 +5,7 @@
 class SoftwareLicense < ApplicationRecord
   include Presentable
 
-  validates :name, presence: true
+  validates :name, presence: true, uniqueness: true
   validates :spdx_identifier, length: { maximum: 255 }
 
   scope :by_name, -> (names) { where(name: names) }

ee/changelogs/unreleased/35938-add-unique-constraint.yml

+---
+title: Add a unique constraint to `software_licenses.name` column
+merge_request: 19840
+author:
+type: fixed

ee/spec/migrations/add_unique_constraint_to_software_licenses_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20191108202723_add_unique_constraint_to_software_licenses.rb')
+
+describe AddUniqueConstraintToSoftwareLicenses, :migration do
+  let(:migration) { described_class.new }
+  let(:projects) { table(:projects) }
+  let(:licenses) { table(:software_licenses) }
+  let(:policies) { table(:software_license_policies) }
+
+  describe "#up" do
+    it 'adds a unique constraint to the name column' do
+      migrate!
+
+      expect(migration.index_exists?(:software_licenses, :name, unique: true)).to be_truthy
+    end
+
+    it 'removes redundant software licenses' do
+      project = projects.create!(name: 'project', namespace_id: 1)
+      other_project = projects.create!(name: 'project', namespace_id: 1)
+
+      apache = licenses.create!(name: 'Apache 2.0')
+      bsd = licenses.create!(name: 'BSD')
+      mit = licenses.create!(name: 'MIT')
+      mit_duplicate = licenses.create!(name: 'MIT')
+
+      apache_policy = policies.create!(software_license_id: apache.id, project_id: project.id)
+      mit_policy = policies.create!(software_license_id: mit.id, project_id: project.id)
+      other_mit_policy = policies.create!(software_license_id: mit_duplicate.id, project_id: other_project.id)
+
+      migrate!
+
+      expect(licenses.all).to contain_exactly(apache, bsd, mit)
+      expect(policies.all).to contain_exactly(apache_policy, mit_policy, other_mit_policy)
+
+      expect(apache_policy.reload.software_license_id).to eql(apache.id)
+      expect(mit_policy.reload.software_license_id).to eql(mit.id)
+      expect(other_mit_policy.reload.software_license_id).to eql(mit.id)
+      expect { mit_duplicate.reload }.to raise_error(ActiveRecord::RecordNotFound)
+    end
+
+    context "when a duplicate record is inserted before adding the unique index" do
+      let!(:mit) { licenses.create!(name: 'MIT') }
+      let!(:mit_duplicate) { licenses.create!(name: 'MIT') }
+      let!(:original_method) { migration.method(:remove_redundant_software_licenses!) }
+
+      before do
+        call_count = 0
+        allow(migration).to receive(:remove_redundant_software_licenses!) do |_|
+          call_count += 1
+          if call_count.odd?
+            raise ActiveRecord::RecordNotUnique
+          else
+            original_method.call
+          end
+        end
+
+        migration.up(attempts: 2)
+      end
+
+      after do
+        migration.down
+      end
+
+      it { expect(licenses.all).to contain_exactly(mit) }
+    end
+  end
+
+  describe "#down" do
+    it 'correctly migrates up and down' do
+      reversible_migration do |x|
+        x.before -> { expect(migration.index_exists?(:software_licenses, :name, unique: true)).to be_falsey }
+        x.after -> { expect(migration.index_exists?(:software_licenses, :name, unique: true)).to be_truthy }
+      end
+    end
+  end
+end

ee/spec/models/software_license_spec.rb

@@ -9,6 +9,7 @@ describe SoftwareLicense do
     it { is_expected.to include_module(Presentable) }
     it { is_expected.to validate_presence_of(:name) }
     it { is_expected.to validate_length_of(:spdx_identifier).is_at_most(255) }
+    it { is_expected.to validate_uniqueness_of(:name) }
   end
 
   describe '.create_policy_for!' do