Don't track MR deployment twice (or more)

This adds a new deployment_id column to deployment_merge_requests table,
with a foreign_key and a uniqueness index on
[environment_id, merge_request_id]

app/models/deployment.rb

@@ -229,7 +229,14 @@ class Deployment < ApplicationRecord
   end
 
   def link_merge_requests(relation)
-    select = relation.select(['merge_requests.id', id]).to_sql
+    # NOTE: relation.select will perform column deduplication,
+    # when id == environment_id it will outputs 2 columns instead of 3
+    # i.e.:
+    # MergeRequest.select(1, 2).to_sql #=> SELECT 1, 2 FROM "merge_requests"
+    # MergeRequest.select(1, 1).to_sql #=> SELECT 1 FROM "merge_requests"
+    select = relation.select('merge_requests.id',
+                             "#{id} as deployment_id",
+                             "#{environment_id} as environment_id").to_sql
 
     # We don't use `Gitlab::Database.bulk_insert` here so that we don't need to
     # first pluck lots of IDs into memory.
@@ -238,7 +245,7 @@ class Deployment < ApplicationRecord
     # for the same deployment, only inserting any missing merge requests.
     DeploymentMergeRequest.connection.execute(<<~SQL)
       INSERT INTO #{DeploymentMergeRequest.table_name}
-      (merge_request_id, deployment_id)
+      (merge_request_id, deployment_id, environment_id)
       #{select}
       ON CONFLICT DO NOTHING
     SQL

changelogs/unreleased/deploy-mr-once-take-2.yml

+---
+title: Don't track MR deployment multiple times
+merge_request: 25537
+author:
+type: fixed

db/migrate/20200219133859_add_environment_id_to_deployment_merge_requests.rb

+# frozen_string_literal: true
+
+class AddEnvironmentIdToDeploymentMergeRequests < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def change
+    add_column :deployment_merge_requests, :environment_id, :integer, null: true
+  end
+end

db/migrate/20200219141307_add_environment_id_fk_to_deployment_merge_requests.rb

+# frozen_string_literal: true
+
+class AddEnvironmentIdFkToDeploymentMergeRequests < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_foreign_key :deployment_merge_requests, :environments, column: :environment_id, on_delete: :cascade
+  end
+
+  def down
+    remove_foreign_key_if_exists :deployment_merge_requests, column: :environment_id
+  end
+end

db/migrate/20200219142522_add_environment_id_merge_request_id_uniq_idx_to_deployment_merge_requests.rb

+# frozen_string_literal: true
+
+class AddEnvironmentIdMergeRequestIdUniqIdxToDeploymentMergeRequests < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :deployment_merge_requests, [:environment_id, :merge_request_id], unique: true, name: 'idx_environment_merge_requests_unique_index'
+  end
+
+  def down
+    remove_concurrent_index_by_name :deployment_merge_requests, 'idx_environment_merge_requests_unique_index'
+  end
+end

db/schema.rb

@@ -1372,7 +1372,9 @@ ActiveRecord::Schema.define(version: 2020_02_26_162723) do
   create_table "deployment_merge_requests", id: false, force: :cascade do |t|
     t.integer "deployment_id", null: false
     t.integer "merge_request_id", null: false
+    t.integer "environment_id"
     t.index ["deployment_id", "merge_request_id"], name: "idx_deployment_merge_requests_unique_index", unique: true
+    t.index ["environment_id", "merge_request_id"], name: "idx_environment_merge_requests_unique_index", unique: true
     t.index ["merge_request_id"], name: "index_deployment_merge_requests_on_merge_request_id"
   end
 
@@ -4719,6 +4721,7 @@ ActiveRecord::Schema.define(version: 2020_02_26_162723) do
   add_foreign_key "deployment_clusters", "clusters", on_delete: :cascade
   add_foreign_key "deployment_clusters", "deployments", on_delete: :cascade
   add_foreign_key "deployment_merge_requests", "deployments", on_delete: :cascade
+  add_foreign_key "deployment_merge_requests", "environments", name: "fk_a064ff4453", on_delete: :cascade
   add_foreign_key "deployment_merge_requests", "merge_requests", on_delete: :cascade
   add_foreign_key "deployments", "clusters", name: "fk_289bba3222", on_delete: :nullify
   add_foreign_key "deployments", "projects", name: "fk_b9a3851b82", on_delete: :cascade

spec/services/deployments/link_merge_requests_service_spec.rb

@@ -133,6 +133,34 @@ describe Deployments::LinkMergeRequestsService do
       expect(deploy.merge_requests).to include(mr1, picked_mr)
     end
 
+    it "doesn't link the same merge_request twice" do
+      create(:merge_request, :merged, merge_commit_sha: mr1_merge_commit_sha,
+             source_project: project)
+
+      picked_mr = create(:merge_request, :merged, merge_commit_sha: '123abc',
+                         source_project: project)
+
+      # the first MR includes c1c67abba which is a cherry-pick of the fake picked_mr merge request
+      create(:track_mr_picking_note, noteable: picked_mr, project: project, commit_id: 'c1c67abbaf91f624347bb3ae96eabe3a1b742478')
+
+      environment = create(:environment, project: project)
+      old_deploy =
+        create(:deployment, :success, project: project, environment: environment)
+
+      # manually linking all the MRs to the old_deploy
+      old_deploy.link_merge_requests(project.merge_requests)
+
+      deploy =
+        create(:deployment, :success, project: project, environment: environment)
+
+      described_class.new(deploy).link_merge_requests_for_range(
+        first_deployment_sha,
+        mr1_merge_commit_sha
+      )
+
+      expect(deploy.merge_requests).to be_empty
+    end
+
     context 'when :track_mr_picking feature flag is disabled' do
       before do
         stub_feature_flags(track_mr_picking: false)