Skip mergeability check when listing MRs in the API

This MR alters the semantics of the mergeability check so the list API
does not enqueue rechecks by default. The previous behaviour can be
restored by use of a REST projection (`with_merge_status_recheck`).

Since the structure of the response remains the same, and even the
content of the returned response, this is not, strictly speaking, a
backwards-incompatible change. However, users who rely on this in the
list API will find they get stale data; rather than going to the single
MR endpoint, the projection gives them a way to specify that they need
the previous behaviour.

Longer-term, we should look at refreshing merge status independently of
any frontend actions. Since we can deduplicate sidekiq jobs now, the
best way to do that is probably to just enqueue a mergeability check
for every affected MR on push, and place strict limits on that sidekiq
queue's concurrency characteristics.

changelogs/unreleased/217455-skip-mergeability-check-on-list-endpoint-by-default.yml

+---
+title: Skip mergeability check when listing MRs in the API
+merge_request: 31890
+author:
+type: performance

doc/api/merge_requests.md

@@ -47,6 +47,7 @@ Parameters:
 | `view`                          | string         | no       | If `simple`, returns the `iid`, URL, title, description, and basic state of merge request                              |
 | `labels`                        | string         | no       | Return merge requests matching a comma separated list of labels. `None` lists all merge requests with no labels. `Any` lists all merge requests with at least one label. `No+Label` (Deprecated) lists all merge requests with no labels. Predefined names are case-insensitive. |
 | `with_labels_details`           | boolean        | no       | If `true`, response will return more details for each label in labels field: `:name`, `:color`, `:description`, `:description_html`, `:text_color`. Default is `false`. Introduced in [GitLab 12.7](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/21413) |
+| `with_merge_status_recheck`     | boolean        | no       | If `true`, this projection requests (but does not guarantee) that the `merge_status` field be recalculated asynchronously. Default is `false`. Introduced in [GitLab 13.0](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/31890) |
 | `created_after`                 | datetime       | no       | Return merge requests created on or after the given time                                                               |
 | `created_before`                | datetime       | no       | Return merge requests created on or before the given time                                                              |
 | `updated_after`                 | datetime       | no       | Return merge requests updated on or after the given time                                                               |
@@ -64,6 +65,13 @@ Parameters:
 | `in`                            | string         | no       | Modify the scope of the `search` attribute. `title`, `description`, or a string joining them with comma. Default is `title,description` |
 | `wip`                           | string         | no       | Filter merge requests against their `wip` status. `yes` to return *only* WIP merge requests, `no` to return *non* WIP merge requests |
 
+NOTE: **Note:**
+[Starting in GitLab 13.0](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/31890),
+listing merge requests may not proactively update the `merge_status` field
+(which also affects the `has_conflicts` field), as this can be an expensive
+operation. If you are interested in the value of these fields from this
+endpoint, set the `with_merge_status_recheck` parameter to `true` in the query.
+
 NOTE: **Note:**
 [Starting in GitLab 12.8](https://gitlab.com/gitlab-org/gitlab/issues/29984),
 when `async_merge_request_check_mergeability` feature flag is enabled, the
@@ -227,6 +235,7 @@ Parameters:
 | `view`                          | string         | no       | If `simple`, returns the `iid`, URL, title, description, and basic state of merge request                                      |
 | `labels`                        | string         | no       | Return merge requests matching a comma separated list of labels. `None` lists all merge requests with no labels. `Any` lists all merge requests with at least one label. `No+Label` (Deprecated) lists all merge requests with no labels. Predefined names are case-insensitive. |
 | `with_labels_details`           | boolean        | no       | If `true`, response will return more details for each label in labels field: `:name`, `:color`, `:description`, `:description_html`, `:text_color`. Default is `false`. Introduced in [GitLab 12.7](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/21413) |
+| `with_merge_status_recheck`     | boolean        | no       | If `true`, this projection requests (but does not guarantee) that the `merge_status` field be recalculated asynchronously. Default is `false`. Introduced in [GitLab 13.0](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/31890) |
 | `created_after`                 | datetime       | no       | Return merge requests created on or after the given time                                                                       |
 | `created_before`                | datetime       | no       | Return merge requests created on or before the given time                                                                      |
 | `updated_after`                 | datetime       | no       | Return merge requests updated on or after the given time                                                                       |
@@ -390,6 +399,7 @@ Parameters:
 | `view`                          | string         | no       | If `simple`, returns the `iid`, URL, title, description, and basic state of merge request                                      |
 | `labels`                        | string         | no       | Return merge requests matching a comma separated list of labels. `None` lists all merge requests with no labels. `Any` lists all merge requests with at least one label. `No+Label` (Deprecated) lists all merge requests with no labels. Predefined names are case-insensitive. |
 | `with_labels_details`           | boolean        | no       | If `true`, response will return more details for each label in labels field: `:name`, `:color`, `:description`, `:description_html`, `:text_color`. Default is `false`. Introduced in [GitLab 12.7](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/21413)|
+| `with_merge_status_recheck`     | boolean        | no       | If `true`, this projection requests (but does not guarantee) that the `merge_status` field be recalculated asynchronously. Default is `false`. Introduced in [GitLab 13.0](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/31890) |
 | `created_after`                 | datetime       | no       | Return merge requests created on or after the given time                                                                       |
 | `created_before`                | datetime       | no       | Return merge requests created on or before the given time                                                                      |
 | `updated_after`                 | datetime       | no       | Return merge requests updated on or after the given time                                                                       |

lib/api/entities/merge_request_basic.rb

@@ -50,8 +50,10 @@ module API
       # use `MergeRequest#mergeable?` instead (boolean).
       # See https://gitlab.com/gitlab-org/gitlab-foss/issues/42344 for more
       # information.
-      expose :merge_status do |merge_request|
-        merge_request.check_mergeability(async: true)
+      #
+      # For list endpoints, we skip the recheck by default, since it's expensive
+      expose :merge_status do |merge_request, options|
+        merge_request.check_mergeability(async: true) unless options[:skip_merge_status_recheck]
         merge_request.public_merge_status
       end
       expose :diff_head_sha, as: :sha

lib/api/helpers/merge_requests_helpers.rb

@@ -27,6 +27,7 @@ module API
                  coerce_with: Validations::Types::LabelsList.coerce,
                  desc: 'Comma-separated list of label names'
         optional :with_labels_details, type: Boolean, desc: 'Return titles of labels and other details', default: false
+        optional :with_merge_status_recheck, type: Boolean, desc: 'Request that stale merge statuses be rechecked asynchronously', default: false
         optional :created_after, type: DateTime, desc: 'Return merge requests created after the specified time'
         optional :created_before, type: DateTime, desc: 'Return merge requests created before the specified time'
         optional :updated_after, type: DateTime, desc: 'Return merge requests updated after the specified time'

lib/api/merge_requests.rb

@@ -93,6 +93,9 @@ module API
           options[:with] = Entities::MergeRequestSimple
         else
           options[:issuable_metadata] = issuable_meta_data(merge_requests, 'MergeRequest', current_user)
+          if Feature.enabled?(:mr_list_api_skip_merge_status_recheck, default_enabled: true)
+            options[:skip_merge_status_recheck] = !declared_params[:with_merge_status_recheck]
+          end
         end
 
         options

spec/requests/api/merge_requests_spec.rb

@@ -66,17 +66,36 @@ describe API::MergeRequests do
       end
 
       context 'when merge request is unchecked' do
+        let(:check_service_class) { MergeRequests::MergeabilityCheckService }
+        let(:mr_entity) { json_response.find { |mr| mr['id'] == merge_request.id } }
+
         before do
           merge_request.mark_as_unchecked!
         end
 
-        it 'checks mergeability asynchronously' do
-          expect_next_instance_of(MergeRequests::MergeabilityCheckService) do |service|
-            expect(service).not_to receive(:execute)
-            expect(service).to receive(:async_execute)
+        context 'with merge status recheck projection' do
+          it 'checks mergeability asynchronously' do
+            expect_next_instance_of(check_service_class) do |service|
+              expect(service).not_to receive(:execute)
+              expect(service).to receive(:async_execute).and_call_original
+            end
+
+            get(api(endpoint_path, user), params: { with_merge_status_recheck: true })
+
+            expect_successful_response_with_paginated_array
+            expect(mr_entity['merge_status']).to eq('checking')
           end
+        end
 
-          get api(endpoint_path, user)
+        context 'without merge status recheck projection' do
+          it 'does not enqueue a merge status recheck' do
+            expect(check_service_class).not_to receive(:new)
+
+            get api(endpoint_path, user)
+
+            expect_successful_response_with_paginated_array
+            expect(mr_entity['merge_status']).to eq('unchecked')
+          end
         end
       end