Merge branch 'ajk-assignee-mr-rest-api-optimization' into 'master'

Optimize for setting MR assignees

See merge request gitlab-org/gitlab!58694

app/graphql/mutations/concerns/mutations/assignable.rb

@@ -19,12 +19,9 @@ module Mutations
 
     def resolve(project_path:, iid:, assignee_usernames:, operation_mode:)
       resource = authorized_find!(project_path: project_path, iid: iid)
+      users = new_assignees(resource, assignee_usernames)
 
-      update_service_class.new(
-        resource.project,
-        current_user,
-        assignee_ids: assignee_ids(resource, assignee_usernames, operation_mode)
-      ).execute(resource)
+      assign!(resource, users, operation_mode)
 
       {
         resource.class.name.underscore.to_sym => resource,
@@ -34,10 +31,20 @@ module Mutations
 
     private
 
-    def assignee_ids(resource, usernames, mode)
-      new = UsersFinder.new(current_user, username: usernames).execute.map(&:id)
+    def assign!(resource, users, operation_mode)
+      update_service_class.new(
+        resource.project,
+        current_user,
+        assignee_ids: assignee_ids(resource, users, operation_mode)
+      ).execute(resource)
+    end
+
+    def new_assignees(resource, usernames)
+      UsersFinder.new(current_user, username: usernames).execute.to_a
+    end
 
-      transform_list(mode, resource, new)
+    def assignee_ids(resource, users, mode)
+      transform_list(mode, resource, users.map(&:id))
     end
 
     def current_assignee_ids(resource)

app/graphql/mutations/issues/set_assignees.rb

@@ -7,6 +7,19 @@ module Mutations
 
       include Assignable
 
+      def assign!(issue, users, mode)
+        permitted, forbidden = users.partition { |u| u.can?(:read_issue, issue) }
+
+        super(issue, permitted, mode)
+
+        forbidden.each do |user|
+          issue.errors.add(
+            :assignees,
+            "Cannot assign #{user.to_reference} to #{issue.to_reference}"
+          )
+        end
+      end
+
       def update_service_class
         ::Issues::UpdateService
       end

app/services/merge_requests/update_assignees_service.rb

@@ -7,15 +7,20 @@ module MergeRequests
     # This saves a lot of queries for irrelevant things that cannot possibly
     # change in the execution of this service.
     def execute(merge_request)
-      return unless current_user&.can?(:update_merge_request, merge_request)
+      return merge_request unless current_user&.can?(:update_merge_request, merge_request)
 
       old_ids = merge_request.assignees.map(&:id)
-      return if old_ids.to_set == update_attrs[:assignee_ids].to_set # no-change
+      new_ids = new_assignee_ids(merge_request)
+      return merge_request if new_ids.size != update_attrs[:assignee_ids].size
+      return merge_request if old_ids.to_set == new_ids.to_set # no-change
 
-      merge_request.update!(**update_attrs)
+      attrs = update_attrs.merge(assignee_ids: new_ids)
+      merge_request.update!(**attrs)
 
       # Defer the more expensive operations (handle_assignee_changes) to the background
       MergeRequests::AssigneesChangeWorker.perform_async(merge_request.id, current_user.id, old_ids)
+
+      merge_request
     end
 
     def handle_assignee_changes(merge_request, old_assignees)
@@ -31,10 +36,33 @@ module MergeRequests
 
     private
 
+    def new_assignee_ids(merge_request)
+      # prime the cache - prevent N+1 lookup during authorization loop.
+      merge_request.project.team.max_member_access_for_user_ids(update_attrs[:assignee_ids])
+      User.id_in(update_attrs[:assignee_ids]).map do |user|
+        if user.can?(:read_merge_request, merge_request)
+          user.id
+        else
+          merge_request.errors.add(
+            :assignees,
+            "Cannot assign #{user.to_reference} to #{merge_request.to_reference}"
+          )
+          nil
+        end
+      end.compact
+    end
+
     def assignee_ids
       params.fetch(:assignee_ids).first(1)
     end
 
+    def params
+      ps = super
+
+      # allow either assignee_id or assignee_ids, preferring assignee_id if passed.
+      { assignee_ids: ps.key?(:assignee_id) ? Array.wrap(ps[:assignee_id]) : ps[:assignee_ids] }
+    end
+
     def update_attrs
       @attrs ||= { updated_at: Time.current, updated_by: current_user, assignee_ids: assignee_ids }
     end

ee/spec/requests/api/graphql/mutations/merge_requests/set_assignees_spec.rb

@@ -109,9 +109,10 @@ RSpec.describe 'Setting assignees of a merge request' do
         #   - SELECT 1 AS one FROM "merge_request_assignees"...
         # Followed by:
         #   - INSERT INTO "merge_request_assignees" ("user_id", "merge_request_id", "created_at")...
+        # On top of which, we have to do an extra authorization fetch
         expect do
           post_graphql_mutation(add_two_assignees, current_user: current_user)
-        end.not_to exceed_query_limit(baseline.count + 2)
+        end.not_to exceed_query_limit(baseline.count + 3)
       end
     end
   end

lib/api/merge_requests.rb

@@ -424,7 +424,13 @@ module API
         mr_params[:force_remove_source_branch] = mr_params.delete(:remove_source_branch) if mr_params.has_key?(:remove_source_branch)
         mr_params = convert_parameters_from_legacy_format(mr_params)
 
-        merge_request = ::MergeRequests::UpdateService.new(user_project, current_user, mr_params).execute(merge_request)
+        service = if mr_params.one? && (mr_params.keys & %i[assignee_id assignee_ids]).one?
+                    ::MergeRequests::UpdateAssigneesService
+                  else
+                    ::MergeRequests::UpdateService
+                  end
+
+        merge_request = service.new(user_project, current_user, mr_params).execute(merge_request)
 
         handle_merge_request_errors!(merge_request)
 

spec/requests/api/graphql/mutations/merge_requests/set_assignees_spec.rb

@@ -80,7 +80,7 @@ RSpec.describe 'Setting assignees of a merge request' do
   end
 
   context 'with assignees already assigned' do
-    let(:db_query_limit) { 38 }
+    let(:db_query_limit) { 39 }
 
     before do
       merge_request.assignees = [assignee2]

spec/requests/api/merge_requests_spec.rb

@@ -2151,6 +2151,23 @@ RSpec.describe API::MergeRequests do
       let(:entity) { merge_request }
     end
 
+    context 'when only assignee_ids are provided' do
+      let(:params) do
+        {
+          assignee_ids: [user2.id]
+        }
+      end
+
+      it 'sets the assignees' do
+        put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user), params: params
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response['assignees']).to contain_exactly(
+          a_hash_including('name' => user2.name)
+        )
+      end
+    end
+
     context 'accepts reviewer_ids' do
       let(:params) do
         {

spec/services/merge_requests/update_assignees_service_spec.rb

@@ -47,6 +47,12 @@ RSpec.describe MergeRequests::UpdateAssigneesService do
           .and change(merge_request, :updated_by).to(user)
       end
 
+      it 'does not update the assignees if they do not have access' do
+        opts[:assignee_ids] = [create(:user).id]
+
+        expect { update_merge_request }.not_to change(merge_request, :assignee_ids)
+      end
+
       it 'is more efficient than using the full update-service' do
         allow(MergeRequests::AssigneesChangeWorker)
           .to receive(:perform_async)

spec/support/shared_examples/graphql/mutations/set_assignees_shared_examples.rb

@@ -22,17 +22,28 @@ RSpec.shared_examples 'an assignable resource' do
                        assignee_usernames: assignee_usernames)
     end
 
-    before do
-      resource.project.add_developer(assignee)
-      resource.project.add_developer(assignee2)
-    end
-
     it 'raises an error if the resource is not accessible to the user' do
       expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
     end
 
+    it 'does not change assignees if the resource is not accessible to the assignees' do
+      resource.project.add_developer(user)
+
+      expect { subject }.not_to change { resource.reload.assignee_ids }
+    end
+
+    it 'returns an operational error if the resource is not accessible to the assignees' do
+      resource.project.add_developer(user)
+
+      result = subject
+
+      expect(result[:errors]).to include a_string_matching(/Cannot assign/)
+    end
+
     context 'when the user can update the resource' do
       before do
+        resource.project.add_developer(assignee)
+        resource.project.add_developer(assignee2)
         resource.project.add_developer(user)
       end