From 3f00efa7c563554aa6c797196a4eeef809a9afc7 Mon Sep 17 00:00:00 2001
From: Vasilii Iakliushin <viakliushin@gitlab.com>
Date: Fri, 28 Feb 2025 11:20:26 +0100
Subject: [PATCH] Add a upper limit for merge request diffs versions
Contributes to https://gitlab.com/gitlab-org/gitlab/-/issues/517497
**Problem**
The upper limit for merge request diffs version is missing.
**Solution**
* Stop creating new diffs versions after reaching the limit.
* Display an error message to the user
---
.../projects/merge_requests_controller.rb | 10 ++++++++
app/models/merge_request.rb | 7 ++++++
.../merge_requests/reload_diffs_service.rb | 3 +++
.../merge_requests_diffs_limit.yml | 9 +++++++
locale/gitlab.pot | 3 +++
.../merge_requests_controller_spec.rb | 24 +++++++++++++++++++
.../reload_diffs_service_spec.rb | 20 ++++++++++++++++
7 files changed, 76 insertions(+)
create mode 100644 config/feature_flags/gitlab_com_derisk/merge_requests_diffs_limit.yml
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 7caa2be78f219..164d10ff52676 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -411,6 +411,8 @@ def show_merge_request
)
end
+ display_max_limit_warning
+
respond_to do |format|
format.html do
# use next to appease Rubocop
@@ -685,6 +687,14 @@ def diffs_stream_resource_url(merge_request, offset, diff_view)
view: diff_view
)
end
+
+ def display_max_limit_warning
+ return unless @merge_request.reached_versions_limit?
+
+ flash[:alert] = format(
+ _("This merge request has reached the maximum limit of %{limit} versions and cannot be updated further. " \
+ "Close this merge request and create a new one instead."), limit: MergeRequest::DIFF_VERSION_LIMIT)
+ end
end
Projects::MergeRequestsController.prepend_mod_with('Projects::MergeRequestsController')
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 3e4cb7be7e61e..858a46031087a 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -39,6 +39,7 @@ class MergeRequest < ApplicationRecord
SORTING_PREFERENCE_FIELD = :merge_requests_sort
CI_MERGE_REQUEST_DESCRIPTION_MAX_LENGTH = 2700
MERGE_LEASE_TIMEOUT = 15.minutes.to_i
+ DIFF_VERSION_LIMIT = 1_000
belongs_to :target_project, class_name: "Project"
belongs_to :source_project, class_name: "Project"
@@ -2454,6 +2455,12 @@ def squash_option
delegate :squash_always?, :squash_never?, :squash_enabled_by_default?, :squash_readonly?, to: :squash_option
+ def reached_versions_limit?
+ return false if Feature.disabled?(:merge_requests_diffs_limit, target_project)
+
+ merge_request_diffs.count >= DIFF_VERSION_LIMIT
+ end
+
private
def merge_base_pipelines
diff --git a/app/services/merge_requests/reload_diffs_service.rb b/app/services/merge_requests/reload_diffs_service.rb
index 2c6ec9333b2d5..ab41ac7526e21 100644
--- a/app/services/merge_requests/reload_diffs_service.rb
+++ b/app/services/merge_requests/reload_diffs_service.rb
@@ -9,6 +9,9 @@ def initialize(merge_request, current_user)
def execute
old_diff_refs = merge_request.diff_refs
+
+ return if merge_request.reached_versions_limit?
+
new_diff = merge_request.create_merge_request_diff
clear_cache(new_diff)
diff --git a/config/feature_flags/gitlab_com_derisk/merge_requests_diffs_limit.yml b/config/feature_flags/gitlab_com_derisk/merge_requests_diffs_limit.yml
new file mode 100644
index 0000000000000..4311b5187f2d8
--- /dev/null
+++ b/config/feature_flags/gitlab_com_derisk/merge_requests_diffs_limit.yml
@@ -0,0 +1,9 @@
+---
+name: merge_requests_diffs_limit
+feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/517497
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/183063
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/521970
+milestone: '17.10'
+group: group::source code
+type: gitlab_com_derisk
+default_enabled: false
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index fe24325e6d19b..eb13e8c2911a2 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -59421,6 +59421,9 @@ msgstr ""
msgid "This merge request does not have codequality reports"
msgstr ""
+msgid "This merge request has reached the maximum limit of %{limit} versions and cannot be updated further. Close this merge request and create a new one instead."
+msgstr ""
+
msgid "This merge request is closed. To apply this suggestion, edit this file directly."
msgstr ""
diff --git a/spec/requests/projects/merge_requests_controller_spec.rb b/spec/requests/projects/merge_requests_controller_spec.rb
index adcaa807b009b..7555669b23b12 100644
--- a/spec/requests/projects/merge_requests_controller_spec.rb
+++ b/spec/requests/projects/merge_requests_controller_spec.rb
@@ -25,6 +25,30 @@
it { is_expected.to have_gitlab_http_status(:not_found) }
end
+
+ context 'when diff version limit is reached' do
+ before do
+ stub_const('MergeRequest::DIFF_VERSION_LIMIT', 1)
+ end
+
+ it 'displays a warning' do
+ get project_merge_request_path(project, merge_request)
+
+ expect(flash[:alert]).to include('This merge request has reached the maximum limit')
+ end
+
+ context 'when "merge_requests_diffs_limit" feature flag is disabled' do
+ before do
+ stub_feature_flags(merge_requests_diffs_limit: false)
+ end
+
+ it 'does not display a warning' do
+ get project_merge_request_path(project, merge_request)
+
+ expect(flash[:alert]).to be_blank
+ end
+ end
+ end
end
describe 'GET #index' do
diff --git a/spec/services/merge_requests/reload_diffs_service_spec.rb b/spec/services/merge_requests/reload_diffs_service_spec.rb
index a6654989374a7..f9bee05c56674 100644
--- a/spec/services/merge_requests/reload_diffs_service_spec.rb
+++ b/spec/services/merge_requests/reload_diffs_service_spec.rb
@@ -32,6 +32,26 @@
subject.execute
end
+ context 'when the number of diff versions reaches the limit' do
+ before do
+ stub_const('MergeRequest::DIFF_VERSION_LIMIT', 1)
+ end
+
+ it 'does not create a new diff' do
+ expect { subject.execute }.not_to change { merge_request.merge_request_diffs.count }
+ end
+
+ context 'when "merge_requests_diffs_limit" feature flag is disabled' do
+ before do
+ stub_feature_flags(merge_requests_diffs_limit: false)
+ end
+
+ it 'creates new merge request diff' do
+ expect { subject.execute }.to change { merge_request.merge_request_diffs.count }.by(1)
+ end
+ end
+ end
+
context 'cache clearing' do
it 'clears the cache for older diffs on the merge request' do
expect_next_instance_of(Gitlab::Diff::FileCollection::MergeRequestDiff) do |instance|
--
GitLab