Merge branch 'ajk-gql-merge-request-reviewers' into 'master'

Add MergeRequest.reviewers

See merge request gitlab-org/gitlab!49707

app/graphql/resolvers/concerns/resolves_merge_requests.rb

@@ -40,6 +40,8 @@ def unconditional_includes
   def preloads
     {
       assignees: [:assignees],
+      reviewers: [:reviewers],
+      participants: MergeRequest.participant_includes,
       labels: [:labels],
       author: [:author],
       merged_at: [:metrics],
@@ -47,6 +49,7 @@ def preloads
       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 @@ class MergeRequestType < BaseObject
           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 @@ def commits_without_merge_commits
     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 @@ def truncate_description_on_import!
   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 @@ def self.wip_title(title)
     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 @@
       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 @@
   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 @@
     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 @@ def query_merge_requests(fields)
     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 @@ def query_merge_requests(fields)
       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 @@ def query_merge_requests(fields)
       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 @@ def execute_query
       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 @@
   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 @@
     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 @@
     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 @@ def sync(value)
     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 @@ def batch(max_queries: nil, &blk)
     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 @@ def all_graphql_fields_for(class_name, parent_types = Set.new, max_depth: 3, exc
     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
-    control_count = ActiveRecord::QueryRecorder.new { execute_query }.count
+    expect(graphql_errors).to be_blank # Sanity check - ex falso quodlibet!
+    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