Add MergeRequest.reviewers GraphQL field

This adds the field `MergeRequest.reviewers`.

Preloads are added to account for the dependent relationships.

Our N+1 testing is improved in a bunch of ways, but removing
confounding factors that are outside the scope of the GraphQL schema,
such as authentication, license loading, and Database read-only checks.

More effort is taken here to correctly isolate the tests from each
other, ensuring we have clean request stores, SQL caches, and
batchloader contexts between runs. We ensure that we are in fact using
the SQL cache.

app/graphql/resolvers/concerns/resolves_merge_requests.rb

@@ -40,6 +40,8 @@ module ResolvesMergeRequests
   def preloads
     {
       assignees: [:assignees],
+      reviewers: [:reviewers],
+      participants: MergeRequest.participant_includes,
       labels: [:labels],
       author: [:author],
       merged_at: [:metrics],
@@ -47,6 +49,7 @@ module ResolvesMergeRequests
       diff_stats_summary: [:metrics],
       approved_by: [:approved_by_users],
       milestone: [:milestone],
+      security_auto_fix: [:author],
       head_pipeline: [:merge_request_diff, { head_pipeline: [:merge_request] }]
     }
   end

app/graphql/types/merge_request_type.rb

@@ -126,10 +126,12 @@ module Types
           description: 'The milestone of the merge request'
     field :assignees, Types::UserType.connection_type, null: true, complexity: 5,
           description: 'Assignees of the merge request'
+    field :reviewers, Types::UserType.connection_type, null: true, complexity: 5,
+          description: 'Users from whom a review has been requested.'
     field :author, Types::UserType, null: true,
           description: 'User who created this merge request'
     field :participants, Types::UserType.connection_type, null: true, complexity: 5,
-          description: 'Participants in the merge request'
+          description: 'Participants in the merge request. This includes the author, assignees, reviewers, and users mentioned in notes.'
     field :subscribed, GraphQL::BOOLEAN_TYPE, method: :subscribed?, null: false, complexity: 5,
           description: 'Indicates if the currently logged in user is subscribed to this merge request'
     field :labels, Types::LabelType.connection_type, null: true, complexity: 5,
@@ -235,6 +237,10 @@ module Types
     def security_auto_fix
       object.author == User.security_bot
     end
+
+    def reviewers
+      object.reviewers if object.allows_reviewers?
+    end
   end
 end
 

app/models/concerns/issuable.rb

@@ -216,6 +216,10 @@ module Issuable
   end
 
   class_methods do
+    def participant_includes
+      [:assignees, :author, { notes: [:author, :award_emoji] }]
+    end
+
     # Searches for records with a matching title.
     #
     # This method uses ILIKE on PostgreSQL.

app/models/merge_request.rb

@@ -493,6 +493,10 @@ class MergeRequest < ApplicationRecord
     work_in_progress?(title) ? title : "Draft: #{title}"
   end
 
+  def self.participant_includes
+    [:reviewers, :award_emoji] + super
+  end
+
   def committers
     @committers ||= commits.committers
   end

changelogs/unreleased/ajk-gql-merge-request-reviewers.yml

+---
+title: Adds MergeRequest.reviewers to GraphQL API
+merge_request: 49707
+author:
+type: changed

doc/api/graphql/reference/gitlab_schema.graphql

@@ -13491,7 +13491,7 @@ type MergeRequest implements CurrentUserTodos & Noteable {
   ): NoteConnection!
 
   """
-  Participants in the merge request
+  Participants in the merge request. This includes the author, assignees, reviewers, and users mentioned in notes.
   """
   participants(
     """
@@ -13586,6 +13586,31 @@ type MergeRequest implements CurrentUserTodos & Noteable {
     full: Boolean = false
   ): String!
 
+  """
+  Users from whom a review has been requested.
+  """
+  reviewers(
+    """
+    Returns the elements in the list that come after the specified cursor.
+    """
+    after: String
+
+    """
+    Returns the elements in the list that come before the specified cursor.
+    """
+    before: String
+
+    """
+    Returns the first _n_ elements from the list.
+    """
+    first: Int
+
+    """
+    Returns the last _n_ elements from the list.
+    """
+    last: Int
+  ): UserConnection
+
   """
   Indicates if the merge request is created by @GitLab-Security-Bot.
   """

doc/api/graphql/reference/gitlab_schema.json

@@ -37252,7 +37252,7 @@
             },
             {
               "name": "participants",
-              "description": "Participants in the merge request",
+              "description": "Participants in the merge request. This includes the author, assignees, reviewers, and users mentioned in notes.",
               "args": [
                 {
                   "name": "after",
@@ -37481,6 +37481,59 @@
               "isDeprecated": false,
               "deprecationReason": null
             },
+            {
+              "name": "reviewers",
+              "description": "Users from whom a review has been requested.",
+              "args": [
+                {
+                  "name": "after",
+                  "description": "Returns the elements in the list that come after the specified cursor.",
+                  "type": {
+                    "kind": "SCALAR",
+                    "name": "String",
+                    "ofType": null
+                  },
+                  "defaultValue": null
+                },
+                {
+                  "name": "before",
+                  "description": "Returns the elements in the list that come before the specified cursor.",
+                  "type": {
+                    "kind": "SCALAR",
+                    "name": "String",
+                    "ofType": null
+                  },
+                  "defaultValue": null
+                },
+                {
+                  "name": "first",
+                  "description": "Returns the first _n_ elements from the list.",
+                  "type": {
+                    "kind": "SCALAR",
+                    "name": "Int",
+                    "ofType": null
+                  },
+                  "defaultValue": null
+                },
+                {
+                  "name": "last",
+                  "description": "Returns the last _n_ elements from the list.",
+                  "type": {
+                    "kind": "SCALAR",
+                    "name": "Int",
+                    "ofType": null
+                  },
+                  "defaultValue": null
+                }
+              ],
+              "type": {
+                "kind": "OBJECT",
+                "name": "UserConnection",
+                "ofType": null
+              },
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
             {
               "name": "securityAutoFix",
               "description": "Indicates if the merge request is created by @GitLab-Security-Bot.",

doc/api/graphql/reference/index.md

@@ -2095,13 +2095,14 @@ Autogenerated return type of MarkAsSpamSnippet.
 | `mergedAt` | Time | Timestamp of when the merge request was merged, null if not merged |
 | `milestone` | Milestone | The milestone of the merge request |
 | `notes` | NoteConnection! | All notes on this noteable |
-| `participants` | UserConnection | Participants in the merge request |
+| `participants` | UserConnection | Participants in the merge request. This includes the author, assignees, reviewers, and users mentioned in notes. |
 | `pipelines` | PipelineConnection | Pipelines for the merge request. Note: for performance reasons, no more than the most recent 500 pipelines will be returned. |
 | `project` | Project! | Alias for target_project |
 | `projectId` | Int! | ID of the merge request project |
 | `rebaseCommitSha` | String | Rebase commit SHA of the merge request |
 | `rebaseInProgress` | Boolean! | Indicates if there is a rebase currently in progress for the merge request |
 | `reference` | String! | Internal reference of the merge request. Returned in shortened format by default |
+| `reviewers` | UserConnection | Users from whom a review has been requested. |
 | `securityAutoFix` | Boolean | Indicates if the merge request is created by @GitLab-Security-Bot. |
 | `shouldBeRebased` | Boolean! | Indicates if the merge request will be rebased |
 | `shouldRemoveSourceBranch` | Boolean | Indicates if the source branch of the merge request will be deleted after merge |

spec/graphql/types/merge_request_type_spec.rb

@@ -25,7 +25,7 @@ RSpec.describe GitlabSchema.types['MergeRequest'] do
       merge_ongoing mergeable_discussions_state web_url
       source_branch_exists target_branch_exists
       upvotes downvotes head_pipeline pipelines task_completion_status
-      milestone assignees participants subscribed labels discussion_locked time_estimate
+      milestone assignees reviewers participants subscribed labels discussion_locked time_estimate
       total_time_spent reference author merged_at commit_count current_user_todos
       conflicts auto_merge_enabled approved_by source_branch_protected
       default_merge_commit_message_with_description squash_on_merge available_auto_merge_strategies

spec/requests/api/graphql/project/merge_request_spec.rb

@@ -9,12 +9,13 @@ RSpec.describe 'getting merge request information nested in a project' do
   let(:current_user) { create(:user) }
   let(:merge_request_graphql_data) { graphql_data['project']['mergeRequest'] }
   let!(:merge_request) { create(:merge_request, source_project: project) }
+  let(:mr_fields) { all_graphql_fields_for('MergeRequest') }
 
   let(:query) do
     graphql_query_for(
       'project',
       { 'fullPath' => project.full_path },
-      query_graphql_field('mergeRequest', iid: merge_request.iid.to_s)
+      query_graphql_field('mergeRequest', { iid: merge_request.iid.to_s }, mr_fields)
     )
   end
 
@@ -43,6 +44,38 @@ RSpec.describe 'getting merge request information nested in a project' do
     expect(merge_request_graphql_data['author']['username']).to eq(merge_request.author.username)
   end
 
+  context 'the merge_request has reviewers' do
+    let(:mr_fields) do
+      <<~SELECT
+      reviewers { nodes { id username } }
+      participants { nodes { id username } }
+      SELECT
+    end
+
+    before do
+      merge_request.reviewers << create_list(:user, 2)
+    end
+
+    it 'includes reviewers' do
+      expected = merge_request.reviewers.map do |r|
+        a_hash_including('id' => global_id_of(r), 'username' => r.username)
+      end
+
+      post_graphql(query, current_user: current_user)
+
+      expect(graphql_data_at(:project, :merge_request, :reviewers, :nodes)).to match_array(expected)
+      expect(graphql_data_at(:project, :merge_request, :participants, :nodes)).to include(*expected)
+    end
+
+    it 'suppresses reviewers if reviewers are not allowed' do
+      stub_feature_flags(merge_request_reviewers: false)
+
+      post_graphql(query, current_user: current_user)
+
+      expect(graphql_data_at(:project, :merge_request, :reviewers)).to be_nil
+    end
+  end
+
   it 'includes diff stats' do
     be_natural = an_instance_of(Integer).and(be >= 0)
 

spec/requests/api/graphql/project/merge_requests_spec.rb

@@ -23,9 +23,7 @@ RSpec.describe 'getting merge request listings nested in a project' do
     graphql_query_for(
       :project,
       { full_path: project.full_path },
-      query_graphql_field(:merge_requests, search_params, [
-        query_graphql_field(:nodes, nil, fields)
-      ])
+      query_nodes(:merge_requests, fields, args: search_params)
     )
   end
 
@@ -50,24 +48,22 @@ RSpec.describe 'getting merge request listings nested in a project' do
       project.repository.expire_branches_cache
     end
 
+    let(:graphql_data) do
+      GitlabSchema.execute(query, context: { current_user: current_user }).to_h['data']
+    end
+
     context 'selecting any single scalar field' do
       where(:field) do
         scalar_fields_of('MergeRequest').map { |name| [name] }
       end
 
       with_them do
-        it_behaves_like 'a working graphql query' do
-          let(:query) do
-            query_merge_requests([:iid, field].uniq)
-          end
-
-          before do
-            post_graphql(query, current_user: current_user)
-          end
-
-          it 'selects the correct MR' do
-            expect(results).to contain_exactly(a_hash_including('iid' => fresh_mr.iid.to_s))
-          end
+        let(:query) do
+          query_merge_requests([:iid, field].uniq)
+        end
+
+        it 'selects the correct MR' do
+          expect(results).to contain_exactly(a_hash_including('iid' => fresh_mr.iid.to_s))
         end
       end
     end
@@ -87,19 +83,13 @@ RSpec.describe 'getting merge request listings nested in a project' do
       end
 
       with_them do
-        it_behaves_like 'a working graphql query' do
-          let(:query) do
-            fld = is_connection ? query_graphql_field(:nodes, nil, [subfield]) : subfield
-            query_merge_requests([:iid, query_graphql_field(field, nil, [fld])])
-          end
-
-          before do
-            post_graphql(query, current_user: current_user)
-          end
-
-          it 'selects the correct MR' do
-            expect(results).to contain_exactly(a_hash_including('iid' => fresh_mr.iid.to_s))
-          end
+        let(:query) do
+          fld = is_connection ? query_graphql_field(:nodes, nil, [subfield]) : subfield
+          query_merge_requests([:iid, query_graphql_field(field, nil, [fld])])
+        end
+
+        it 'selects the correct MR' do
+          expect(results).to contain_exactly(a_hash_including('iid' => fresh_mr.iid.to_s))
         end
       end
     end
@@ -254,6 +244,115 @@ RSpec.describe 'getting merge request listings nested in a project' do
 
       include_examples 'N+1 query check'
     end
+
+    context 'when requesting reviewers' do
+      let(:requested_fields) { ['reviewers { nodes { username } }'] }
+
+      before do
+        merge_request_a.reviewers << create(:user)
+        merge_request_a.reviewers << create(:user)
+        merge_request_c.reviewers << create(:user)
+      end
+
+      it 'returns the reviewers' do
+        execute_query
+
+        expect(results).to include a_hash_including('reviewers' => {
+          'nodes' => match_array(merge_request_a.reviewers.map do |r|
+            a_hash_including('username' => r.username)
+          end)
+        })
+      end
+
+      context 'the feature flag is disabled' do
+        before do
+          stub_feature_flags(merge_request_reviewers: false)
+        end
+
+        it 'does not return reviewers' do
+          execute_query
+
+          expect(results).to all(match a_hash_including('reviewers' => be_nil))
+        end
+      end
+
+      include_examples 'N+1 query check'
+    end
+  end
+
+  describe 'performance' do
+    let(:mr_fields) do
+      <<~SELECT
+      assignees { nodes { username } }
+      reviewers { nodes { username } }
+      participants { nodes { username } }
+      headPipeline { status }
+      SELECT
+    end
+
+    let(:query) do
+      <<~GQL
+        query($first: Int) {
+          project(fullPath: "#{project.full_path}") {
+            mergeRequests(first: $first) {
+              nodes { #{mr_fields} }
+            }
+          }
+        }
+      GQL
+    end
+
+    before_all do
+      project.add_developer(current_user)
+      mrs = create_list(:merge_request, 10, :closed, :with_head_pipeline,
+                        source_project: project,
+                        author: current_user)
+      mrs.each do |mr|
+        mr.assignees << create(:user)
+        mr.assignees << current_user
+        mr.reviewers << create(:user)
+        mr.reviewers << current_user
+      end
+    end
+
+    before do
+      # Confounding factor: makes DB calls in EE
+      allow(Gitlab::Database).to receive(:read_only?).and_return(false)
+    end
+
+    def run_query(number)
+      # Ensure that we have a fresh request store and batch-context between runs
+      result = run_with_clean_state(query,
+        context: { current_user: current_user },
+        variables: { first: number }
+      )
+
+      graphql_dig_at(result.to_h, :data, :project, :merge_requests, :nodes)
+    end
+
+    def user_collection
+      { 'nodes' => all(match(a_hash_including('username' => be_present))) }
+    end
+
+    it 'returns appropriate results' do
+      mrs = run_query(2)
+
+      expect(mrs.size).to eq(2)
+      expect(mrs).to all(
+        match(
+          a_hash_including(
+            'assignees' => user_collection,
+            'reviewers' => user_collection,
+            'participants' => user_collection,
+            'headPipeline' => { 'status' => be_present }
+          )))
+    end
+
+    it 'can lookahead to eliminate N+1 queries' do
+      baseline = ActiveRecord::QueryRecorder.new { run_query(1) }
+
+      expect { run_query(10) }.not_to exceed_query_limit(baseline)
+    end
   end
 
   describe 'sorting and pagination' do

spec/requests/api/graphql/project_query_spec.rb

@@ -6,26 +6,21 @@ RSpec.describe 'getting project information' do
   include GraphqlHelpers
 
   let_it_be(:group) { create(:group) }
-  let_it_be(:project) { create(:project, :repository, group: group) }
+  let_it_be(:project, reload: true) { create(:project, :repository, group: group) }
   let_it_be(:current_user) { create(:user) }
-  let(:fields) { all_graphql_fields_for(Project, max_depth: 2, excluded: %w(jiraImports services)) }
+  let(:project_fields) { all_graphql_fields_for('project'.to_s.classify, max_depth: 1) }
 
   let(:query) do
-    graphql_query_for(:project, { full_path: project.full_path }, fields)
+    graphql_query_for(:project, { full_path: project.full_path }, project_fields)
   end
 
   context 'when the user has full access to the project' do
-    let(:full_access_query) do
-      graphql_query_for(:project, { full_path: project.full_path },
-                        all_graphql_fields_for('Project', max_depth: 2))
-    end
-
     before do
       project.add_maintainer(current_user)
     end
 
     it 'includes the project', :use_clean_rails_memory_store_caching, :request_store do
-      post_graphql(full_access_query, current_user: current_user)
+      post_graphql(query, current_user: current_user)
 
       expect(graphql_data['project']).not_to be_nil
     end
@@ -49,12 +44,12 @@ RSpec.describe 'getting project information' do
     end
 
     context 'when there are pipelines present' do
+      let(:project_fields) { query_nodes(:pipelines) }
+
       before do
         create(:ci_pipeline, project: project)
       end
 
-      let(:fields) { query_nodes(:pipelines) }
-
       it 'is included in the pipelines connection' do
         post_graphql(query, current_user: current_user)
 
@@ -108,55 +103,6 @@ RSpec.describe 'getting project information' do
     end
   end
 
-  describe 'performance' do
-    before_all do
-      project.add_developer(current_user)
-      mrs = create_list(:merge_request, 10, :closed, :with_head_pipeline,
-                        source_project: project,
-                        author: current_user)
-      mrs.each do |mr|
-        mr.assignees << create(:user)
-        mr.assignees << current_user
-      end
-    end
-
-    def run_query(number)
-      q = <<~GQL
-        query {
-          project(fullPath: "#{project.full_path}") {
-            mergeRequests(first: #{number}) {
-              nodes {
-                assignees { nodes { username } }
-                headPipeline { status }
-              }
-            }
-          }
-        }
-      GQL
-
-      post_graphql(q, current_user: current_user)
-    end
-
-    it 'returns appropriate results' do
-      run_query(2)
-
-      mrs = graphql_data.dig('project', 'mergeRequests', 'nodes')
-
-      expect(mrs.size).to eq(2)
-      expect(mrs).to all(
-        match(
-          a_hash_including(
-            'assignees' => { 'nodes' => all(match(a_hash_including('username' => be_present))) },
-            'headPipeline' => { 'status' => be_present }
-          )))
-    end
-
-    it 'can lookahead to eliminate N+1 queries' do
-      baseline = ActiveRecord::QueryRecorder.new { run_query(1) }
-      expect { run_query(10) }.not_to exceed_query_limit(baseline)
-    end
-  end
-
   context 'when the user does not have access to the project' do
     it 'returns an empty field' do
       post_graphql(query, current_user: current_user)

spec/support/helpers/graphql_helpers.rb

@@ -67,14 +67,16 @@ module GraphqlHelpers
     end
   end
 
+  def with_clean_batchloader_executor(&block)
+    BatchLoader::Executor.ensure_current
+    yield
+  ensure
+    BatchLoader::Executor.clear_current
+  end
+
   # Runs a block inside a BatchLoader::Executor wrapper
   def batch(max_queries: nil, &blk)
-    wrapper = proc do
-      BatchLoader::Executor.ensure_current
-      yield
-    ensure
-      BatchLoader::Executor.clear_current
-    end
+    wrapper = -> { with_clean_batchloader_executor(&blk) }
 
     if max_queries
       result = nil
@@ -85,6 +87,32 @@ module GraphqlHelpers
     end
   end
 
+  # Use this when writing N+1 tests.
+  #
+  # It does not use the controller, so it avoids confounding factors due to
+  # authentication (token set-up, license checks)
+  # It clears the request store, rails cache, and BatchLoader Executor between runs.
+  def run_with_clean_state(query, **args)
+    ::Gitlab::WithRequestStore.with_request_store do
+      with_clean_rails_cache do
+        with_clean_batchloader_executor do
+          ::GitlabSchema.execute(query, **args)
+        end
+      end
+    end
+  end
+
+  # Basically a combination of use_sql_query_cache and use_clean_rails_memory_store_caching,
+  # but more fine-grained, suitable for comparing two runs in the same example.
+  def with_clean_rails_cache(&blk)
+    caching_store = Rails.cache
+    Rails.cache = ActiveSupport::Cache::MemoryStore.new
+
+    ActiveRecord::Base.cache(&blk)
+  ensure
+    Rails.cache = caching_store
+  end
+
   # BatchLoader::GraphQL returns a wrapper, so we need to :sync in order
   # to get the actual values
   def batch_sync(max_queries: nil, &blk)
@@ -245,7 +273,7 @@ module GraphqlHelpers
     return if max_depth <= 0
 
     allow_unlimited_graphql_complexity
-    allow_unlimited_graphql_depth
+    allow_unlimited_graphql_depth if max_depth > 1
     allow_high_graphql_recursion
     allow_high_graphql_transaction_threshold
 

spec/support/shared_examples/graphql/projects/merge_request_n_plus_one_query_examples.rb

@@ -2,10 +2,12 @@
 shared_examples 'N+1 query check' do
   it 'prevents N+1 queries' do
     execute_query # "warm up" to prevent undeterministic counts
+    expect(graphql_errors).to be_blank # Sanity check - ex falso quodlibet!
 
-    control_count = ActiveRecord::QueryRecorder.new { execute_query }.count
+    control = ActiveRecord::QueryRecorder.new { execute_query }
+    expect(control.count).to be > 0
 
     search_params[:iids] << extra_iid_for_second_query
-    expect { execute_query }.not_to exceed_query_limit(control_count)
+    expect { execute_query }.not_to exceed_query_limit(control)
   end
 end