Address API and import/export backward compatibility

This commit provides the required code for making
creation and update of MRs with the `assignee_id`
params backward compatible.

Additionally, it provides the required interface
for the MergeRequest model to make old and new
exported GitLab versions handle the `assignee_id`
well.

app/models/concerns/deprecated_assignee.rb

+# frozen_string_literal: true
+
+# This module handles backward compatibility for import/export of Merge Requests after
+# multiple assignees feature was introduced. Also, it handles the scenarios where
+# the #26496 background migration hasn't finished yet.
+# Ideally, most of this code should be removed at #59457.
+module DeprecatedAssignee
+  extend ActiveSupport::Concern
+
+  def assignee_ids=(ids)
+    nullify_deprecated_assignee
+    super
+  end
+
+  def assignees=(users)
+    nullify_deprecated_assignee
+    super
+  end
+
+  def assignee_id=(id)
+    self.assignee_ids = Array(id)
+  end
+
+  def assignee=(user)
+    self.assignees = Array(user)
+  end
+
+  def assignee
+    assignees.first
+  end
+
+  def assignee_id
+    assignee_ids.first
+  end
+
+  def assignee_ids
+    if Gitlab::Database.read_only? && pending_assignees_population?
+      return Array(deprecated_assignee_id)
+    end
+
+    update_assignees_relation
+    super
+  end
+
+  def assignees
+    if Gitlab::Database.read_only? && pending_assignees_population?
+      return User.where(id: deprecated_assignee_id)
+    end
+
+    update_assignees_relation
+    super
+  end
+
+  private
+
+  # This will make the background migration process quicker (#26496) as it'll have less
+  # assignee_id rows to look through.
+  def nullify_deprecated_assignee
+    return unless persisted? && Gitlab::Database.read_only?
+
+    update_column(:assignee_id, nil)
+  end
+
+  # This code should be removed in the clean-up phase of the
+  # background migration (#59457).
+  def pending_assignees_population?
+    persisted? && deprecated_assignee_id && merge_request_assignees.empty?
+  end
+
+  # If there's an assignee_id and no relation, it means the background
+  # migration at #26496 didn't reach this merge request yet.
+  # This code should be removed in the clean-up phase of the
+  # background migration (#59457).
+  def update_assignees_relation
+    if pending_assignees_population?
+      transaction do
+        merge_request_assignees.create!(user_id: deprecated_assignee_id, merge_request_id: id)
+        update_column(:assignee_id, nil)
+      end
+    end
+  end
+
+  def deprecated_assignee_id
+    read_attribute(:assignee_id)
+  end
+end

app/models/merge_request.rb

@@ -16,6 +16,7 @@ class MergeRequest < ApplicationRecord
   include LabelEventable
   include ReactiveCaching
   include FromUnion
+  include DeprecatedAssignee
 
   self.reactive_cache_key = ->(model) { [model.project.id, model.iid] }
   self.reactive_cache_refresh_interval = 10.minutes
@@ -329,30 +330,6 @@ class MergeRequest < ApplicationRecord
     Gitlab::HookData::MergeRequestBuilder.new(self).build
   end
 
-  # Used by APIs to keep backward compatibility
-  def assignee
-    assignees.first
-  end
-
-  def assignees
-    # We're willing to write in the DB if we can't find the relation
-    # (i.e. migration at #26496 is not finished yet).
-    return super if Gitlab::Database.read_only?
-
-    # If there's an assignee_id and no relation, it means the background
-    # migration at #26496 didn't reach this merge request yet.
-    # This code should be removed in the clean-up phase of the
-    # background migration (#59457).
-    if persisted? && assignee_id && merge_request_assignees.empty?
-      transaction do
-        merge_request_assignees.create(user_id: assignee_id, merge_request_id: id)
-        update_column(:assignee_id, nil)
-      end
-    end
-
-    super
-  end
-
   # `from` argument can be a Namespace or Project.
   def to_reference(from = nil, full: false)
     reference = "#{self.class.reference_prefix}#{iid}"

ee/spec/requests/api/merge_requests_spec.rb

@@ -20,11 +20,49 @@ describe API::MergeRequests do
   end
 
   describe 'PUT /projects/:id/merge_requests' do
-    context 'when updating existing approval rules' do
-      def update_merge_request(params)
-        put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user), params: params
+    def update_merge_request(params)
+      put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user), params: params
+    end
+
+    context 'multiple assignees' do
+      let(:other_user) { create(:user) }
+      let(:params) do
+        { assignee_ids: [user.id, other_user.id] }
+      end
+
+      context 'when licensed' do
+        before do
+          stub_licensed_features(multiple_merge_request_assignees: true)
+        end
+
+        it 'creates merge request with multiple assignees' do
+          update_merge_request(params)
+
+          expect(response).to have_gitlab_http_status(200)
+          expect(json_response['assignees'].size).to eq(2)
+          expect(json_response['assignees'].first['name']).to eq(user.name)
+          expect(json_response['assignees'].second['name']).to eq(other_user.name)
+          expect(json_response.dig('assignee', 'name')).to eq(user.name)
+        end
       end
 
+      context 'when not licensed' do
+        before do
+          stub_licensed_features(multiple_merge_request_assignees: false)
+        end
+
+        it 'creates merge request with a single assignee' do
+          update_merge_request(params)
+
+          expect(response).to have_gitlab_http_status(200)
+          expect(json_response['assignees'].size).to eq(1)
+          expect(json_response['assignees'].first['name']).to eq(user.name)
+          expect(json_response.dig('assignee', 'name')).to eq(user.name)
+        end
+      end
+    end
+
+    context 'when updating existing approval rules' do
       let!(:rule) { create(:approval_merge_request_rule, merge_request: merge_request, approvals_required: 1) }
 
       it 'is successful' do
@@ -58,6 +96,40 @@ describe API::MergeRequests do
       defaults = defaults.merge(args)
       post api("/projects/#{project.id}/merge_requests", user), params: defaults
     end
+
+    context 'multiple assignees' do
+      context 'when licensed' do
+        before do
+          stub_licensed_features(multiple_merge_request_assignees: true)
+        end
+
+        it 'creates merge request with multiple assignees' do
+          create_merge_request(assignee_ids: [user.id, user2.id])
+
+          expect(response).to have_gitlab_http_status(201)
+          expect(json_response['assignees'].size).to eq(2)
+          expect(json_response['assignees'].first['name']).to eq(user.name)
+          expect(json_response['assignees'].second['name']).to eq(user2.name)
+          expect(json_response.dig('assignee', 'name')).to eq(user.name)
+        end
+      end
+
+      context 'when not licensed' do
+        before do
+          stub_licensed_features(multiple_merge_request_assignees: false)
+        end
+
+        it 'creates merge request with a single assignee' do
+          create_merge_request(assignee_ids: [user.id, user2.id])
+
+          expect(response).to have_gitlab_http_status(201)
+          expect(json_response['assignees'].size).to eq(1)
+          expect(json_response['assignees'].first['name']).to eq(user.name)
+          expect(json_response.dig('assignee', 'name')).to eq(user.name)
+        end
+      end
+    end
+
     context 'between branches projects' do
       it "returns merge_request" do
         create_merge_request(squash: true)
@@ -143,9 +215,19 @@ describe API::MergeRequests do
       create(:merge_request_with_approver, :simple, author: user, source_project: project, target_project: project, source_branch: 'other-branch')
     end
 
-    let(:another_user) {}
+    context 'filter merge requests by assignee ID' do
+      let!(:merge_request2) do
+        create(:merge_request, :simple, assignees: [user2], source_project: project, target_project: project, source_branch: 'other-branch-2')
+      end
+
+      it 'returns merge requests with given assignee ID' do
+        get api('/merge_requests', user), params: { assignee_id: user2.id }
+
+        expect_response_contain_exactly(merge_request, merge_request2)
+      end
+    end
 
-    context 'request merge requests' do
+    context 'filter merge requests by approver IDs' do
       before do
         get api('/merge_requests', user), params: { approver_ids: approvers_param, scope: :all }
       end

lib/api/merge_requests.rb

@@ -20,6 +20,7 @@ module API
     def self.update_params_at_least_one_of
       %i[
         assignee_id
+        assignee_ids
         description
         labels
         milestone_id
@@ -186,6 +187,7 @@ module API
         params :optional_params do
           optional :description, type: String, desc: 'The description of the merge request'
           optional :assignee_id, type: Integer, desc: 'The ID of a user to assign the merge request'
+          optional :assignee_ids, type: Array[Integer], desc: 'The array of user IDs to assign issue'
           optional :milestone_id, type: Integer, desc: 'The ID of a milestone to assign the merge request'
           optional :labels, type: Array[String], coerce_with: Validations::Types::LabelsList.coerce, desc: 'Comma-separated list of label names'
           optional :remove_source_branch, type: Boolean, desc: 'Remove source branch when merging'
@@ -233,6 +235,7 @@ module API
 
         mr_params = declared_params(include_missing: false)
         mr_params[:force_remove_source_branch] = mr_params.delete(:remove_source_branch)
+        mr_params = convert_parameters_from_legacy_format(mr_params)
 
         merge_request = ::MergeRequests::CreateService.new(user_project, current_user, mr_params).execute
 
@@ -336,6 +339,7 @@ module API
 
         mr_params = declared_params(include_missing: false)
         mr_params[:force_remove_source_branch] = mr_params.delete(:remove_source_branch) if mr_params[:remove_source_branch].present?
+        mr_params = convert_parameters_from_legacy_format(mr_params)
 
         merge_request = ::MergeRequests::UpdateService.new(user_project, current_user, mr_params).execute(merge_request)
 

lib/gitlab/import_export/import_export.yml

@@ -47,7 +47,6 @@ project_tree:
     - label_links:
       - label:
           :priorities
-    - :merge_request_assignees
     - milestone:
       - events:
         - :push_event_payload

spec/models/concerns/deprecated_assignee_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe DeprecatedAssignee do
+  let(:user) { create(:user) }
+
+  describe '#assignee_id=' do
+    it 'creates the merge_request_assignees relation' do
+      merge_request = create(:merge_request, assignee_id: user.id)
+
+      merge_request.reload
+
+      expect(merge_request.merge_request_assignees.count).to eq(1)
+    end
+
+    it 'nullifies the assignee_id column' do
+      merge_request = create(:merge_request, assignee_id: user.id)
+
+      merge_request.reload
+
+      expect(merge_request.read_attribute(:assignee_id)).to be_nil
+    end
+
+    context 'when relation already exists' do
+      it 'overwrites existing assignees' do
+        other_user = create(:user)
+        merge_request = create(:merge_request, assignee_id: nil)
+        merge_request.merge_request_assignees.create!(user_id: user.id)
+        merge_request.merge_request_assignees.create!(user_id: other_user.id)
+
+        expect { merge_request.update!(assignee_id: other_user.id) }
+          .to change { merge_request.reload.merge_request_assignees.count }
+          .from(2).to(1)
+      end
+    end
+  end
+
+  describe '#assignee=' do
+    it 'creates the merge_request_assignees relation' do
+      merge_request = create(:merge_request, assignee: user)
+
+      merge_request.reload
+
+      expect(merge_request.merge_request_assignees.count).to eq(1)
+    end
+
+    it 'nullifies the assignee_id column' do
+      merge_request = create(:merge_request, assignee: user)
+
+      merge_request.reload
+
+      expect(merge_request.read_attribute(:assignee_id)).to be_nil
+    end
+
+    context 'when relation already exists' do
+      it 'overwrites existing assignees' do
+        other_user = create(:user)
+        merge_request = create(:merge_request, assignee: nil)
+        merge_request.merge_request_assignees.create!(user_id: user.id)
+        merge_request.merge_request_assignees.create!(user_id: other_user.id)
+
+        expect { merge_request.update!(assignee: other_user) }
+          .to change { merge_request.reload.merge_request_assignees.count }
+          .from(2).to(1)
+      end
+    end
+  end
+
+  describe '#assignee_id' do
+    it 'returns the first assignee ID' do
+      other_user = create(:user)
+      merge_request = create(:merge_request, assignees: [user, other_user])
+
+      merge_request.reload
+
+      expect(merge_request.assignee_id).to eq(merge_request.assignee_ids.first)
+    end
+  end
+
+  describe '#assignees' do
+    context 'when assignee_id exists and there is no relation' do
+      it 'creates the relation' do
+        merge_request = create(:merge_request, assignee_id: nil)
+        merge_request.update_column(:assignee_id, user.id)
+
+        expect { merge_request.assignees }.to change { merge_request.merge_request_assignees.count }.from(0).to(1)
+      end
+
+      it 'nullifies the assignee_id' do
+        merge_request = create(:merge_request, assignee_id: nil)
+        merge_request.update_column(:assignee_id, user.id)
+
+        expect { merge_request.assignees }
+          .to change { merge_request.read_attribute(:assignee_id) }
+          .from(user.id).to(nil)
+      end
+    end
+
+    context 'when DB is read-only' do
+      before do
+        allow(Gitlab::Database).to receive(:read_only?) { true }
+      end
+
+      it 'returns a users relation' do
+        merge_request = create(:merge_request, assignee_id: user.id)
+
+        expect(merge_request.assignees).to be_a(ActiveRecord::Relation)
+        expect(merge_request.assignees).to eq([user])
+      end
+
+      it 'returns an empty relation if no assignee_id is set' do
+        merge_request = create(:merge_request, assignee_id: nil)
+
+        expect(merge_request.assignees).to be_a(ActiveRecord::Relation)
+        expect(merge_request.assignees).to eq([])
+      end
+    end
+  end
+
+  describe '#assignee_ids' do
+    context 'when assignee_id exists and there is no relation' do
+      it 'creates the relation' do
+        merge_request = create(:merge_request, assignee_id: nil)
+        merge_request.update_column(:assignee_id, user.id)
+
+        expect { merge_request.assignee_ids }.to change { merge_request.merge_request_assignees.count }.from(0).to(1)
+      end
+
+      it 'nullifies the assignee_id' do
+        merge_request = create(:merge_request, assignee_id: nil)
+        merge_request.update_column(:assignee_id, user.id)
+
+        expect { merge_request.assignee_ids }
+          .to change { merge_request.read_attribute(:assignee_id) }
+          .from(user.id).to(nil)
+      end
+    end
+
+    context 'when DB is read-only' do
+      before do
+        allow(Gitlab::Database).to receive(:read_only?) { true }
+      end
+
+      it 'returns a list of user IDs' do
+        merge_request = create(:merge_request, assignee_id: user.id)
+
+        expect(merge_request.assignee_ids).to be_a(Array)
+        expect(merge_request.assignee_ids).to eq([user.id])
+      end
+
+      it 'returns an empty relation if no assignee_id is set' do
+        merge_request = create(:merge_request, assignee_id: nil)
+
+        expect(merge_request.assignee_ids).to be_a(Array)
+        expect(merge_request.assignee_ids).to eq([])
+      end
+    end
+  end
+end

spec/models/merge_request_spec.rb

@@ -523,52 +523,6 @@ describe MergeRequest do
     end
   end
 
-  describe '#assignees' do
-    let(:user) { create(:user) }
-
-    context 'when assignee_id exists and relation is not empty' do
-      it 'does not create assignees relation' do
-        merge_request = create(:merge_request, assignee_id: user.id)
-        merge_request.merge_request_assignees.create(user_id: user.id)
-
-        expect { merge_request.assignees }.not_to change { merge_request.reload.assignee_ids.size }
-      end
-    end
-
-    context 'when assignee_id exists and relation is empty' do
-      it 'creates assignees relation' do
-        merge_request = create(:merge_request, assignee_id: user.id)
-
-        expect { merge_request.assignees }.to change { merge_request.reload.assignee_ids.size }.from(0).to(1)
-        expect(merge_request.assignees.first).to eq(user)
-      end
-
-      it 'nullifies assignee_id' do
-        merge_request = create(:merge_request, assignee_id: user.id)
-
-        expect { merge_request.assignees }.to change { merge_request.reload.assignee_id }.to(nil)
-      end
-    end
-
-    context 'when assignee_id does not exists' do
-      it 'does not create assignees relation' do
-        merge_request = create(:merge_request, assignee_id: nil)
-
-        expect { merge_request.assignees }.not_to change { merge_request.reload.assignee_ids.size }
-      end
-    end
-
-    context 'when DB is read-only' do
-      it 'does not create assignees relation' do
-        allow(Gitlab::Database).to receive(:read_only?) { true }
-
-        merge_request = create(:merge_request, assignee_id: user.id)
-
-        expect { merge_request.assignees }.not_to change { merge_request.reload.assignee_ids.size }
-      end
-    end
-  end
-
   describe '#to_reference' do
     let(:project) { build(:project, name: 'sample-project') }
     let(:merge_request) { build(:merge_request, target_project: project, iid: 1) }

spec/requests/api/merge_requests_spec.rb

@@ -5,6 +5,7 @@ describe API::MergeRequests do
 
   let(:base_time)   { Time.now }
   set(:user)        { create(:user) }
+  set(:user2)       { create(:user) }
   set(:admin)       { create(:user, :admin) }
   let(:project)     { create(:project, :public, :repository, creator: user, namespace: user.namespace, only_allow_merge_if_pipeline_succeeds: false) }
   let(:milestone)   { create(:milestone, title: '1.0.0', project: project) }
@@ -20,6 +21,9 @@ describe API::MergeRequests do
 
   before do
     project.add_reporter(user)
+    project.add_reporter(user2)
+
+    stub_licensed_features(multiple_merge_request_assignees: false)
   end
 
   shared_context 'with labels' do
@@ -730,7 +734,7 @@ describe API::MergeRequests do
 
   describe "GET /projects/:id/merge_requests/:merge_request_iid" do
     it 'matches json schema' do
-      merge_request = create(:merge_request, :with_test_reports, milestone: milestone1, author: user, assignee: user, source_project: project, target_project: project, title: "Test", created_at: base_time)
+      merge_request = create(:merge_request, :with_test_reports, milestone: milestone1, author: user, assignees: [user], source_project: project, target_project: project, title: "Test", created_at: base_time)
       get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user)
 
       expect(response).to have_gitlab_http_status(200)
@@ -1005,6 +1009,71 @@ describe API::MergeRequests do
   end
 
   describe 'POST /projects/:id/merge_requests' do
+    context 'support for deprecated assignee_id' do
+      let(:params) do
+        {
+          title: 'Test merge request',
+          source_branch: 'feature_conflict',
+          target_branch: 'master',
+          author_id: user.id,
+          assignee_id: user2.id
+        }
+      end
+
+      it 'creates a new merge request' do
+        post api("/projects/#{project.id}/merge_requests", user), params: params
+
+        expect(response).to have_gitlab_http_status(201)
+        expect(json_response['title']).to eq('Test merge request')
+        expect(json_response['assignee']['name']).to eq(user2.name)
+        expect(json_response['assignees'].first['name']).to eq(user2.name)
+      end
+
+      it 'creates a new merge request when assignee_id is empty' do
+        params[:assignee_id] = ''
+
+        post api("/projects/#{project.id}/merge_requests", user), params: params
+
+        expect(response).to have_gitlab_http_status(201)
+        expect(json_response['title']).to eq('Test merge request')
+        expect(json_response['assignee']).to be_nil
+      end
+
+      it 'filters assignee_id of unauthorized user' do
+        private_project = create(:project, :private, :repository)
+        another_user = create(:user)
+        private_project.add_maintainer(user)
+        params[:assignee_id] = another_user.id
+
+        post api("/projects/#{private_project.id}/merge_requests", user), params: params
+
+        expect(response).to have_gitlab_http_status(201)
+        expect(json_response['assignee']).to be_nil
+      end
+    end
+
+    context 'single assignee restrictions' do
+      let(:params) do
+        {
+          title: 'Test merge request',
+          source_branch: 'feature_conflict',
+          target_branch: 'master',
+          author_id: user.id,
+          assignee_ids: [user.id, user2.id]
+        }
+      end
+
+      it 'creates a new project merge request with no more than one assignee' do
+        post api("/projects/#{project.id}/merge_requests", user), params: params
+
+        expect(response).to have_gitlab_http_status(201)
+        expect(json_response['title']).to eq('Test merge request')
+        expect(json_response['assignees'].count).to eq(1)
+        expect(json_response['assignees'].first['name']).to eq(user.name)
+        expect(json_response.dig('assignee', 'name')).to eq(user.name)
+      end
+    end
+
     context 'between branches projects' do
       context 'different labels' do
         let(:params) do
@@ -1595,6 +1664,19 @@ describe API::MergeRequests do
       expect(json_response['force_remove_source_branch']).to be_truthy
     end
 
+    it 'filters assignee_id of unauthorized user' do
+      private_project = create(:project, :private, :repository)
+      mr = create(:merge_request, source_project: private_project, target_project: private_project)
+      another_user = create(:user)
+      private_project.add_maintainer(user)
+      params = { assignee_id: another_user.id }
+
+      put api("/projects/#{private_project.id}/merge_requests/#{mr.iid}", user), params: params
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(json_response['assignee']).to be_nil
+    end
+
     context 'when updating labels' do
       it 'allows special label names' do
         put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user),