Merge branch '31063-ensure-diff-collection-limits-are-monitorable' into 'master'

Resolve "Ensure diff collection limits are configurable" [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!56722

app/helpers/application_settings_helper.rb

@@ -338,6 +338,8 @@ module ApplicationSettingsHelper
       :version_check_enabled,
       :web_ide_clientside_preview_enabled,
       :diff_max_patch_bytes,
+      :diff_max_files,
+      :diff_max_lines,
       :commit_email_hostname,
       :protected_ci_variables,
       :local_markdown_version,

app/models/application_setting.rb

@@ -273,6 +273,18 @@ class ApplicationSetting < ApplicationRecord
                             greater_than_or_equal_to: Gitlab::Git::Diff::DEFAULT_MAX_PATCH_BYTES,
                             less_than_or_equal_to: Gitlab::Git::Diff::MAX_PATCH_BYTES_UPPER_BOUND }
 
+  validates :diff_max_files,
+            presence: true,
+            numericality: { only_integer: true,
+                            greater_than_or_equal_to: Commit::DEFAULT_MAX_DIFF_FILES_SETTING,
+                            less_than_or_equal_to: Commit::MAX_DIFF_FILES_SETTING_UPPER_BOUND }
+
+  validates :diff_max_lines,
+            presence: true,
+            numericality: { only_integer: true,
+                            greater_than_or_equal_to: Commit::DEFAULT_MAX_DIFF_LINES_SETTING,
+                            less_than_or_equal_to: Commit::MAX_DIFF_LINES_SETTING_UPPER_BOUND }
+
   validates :user_default_internal_regex, js_regex: true, allow_nil: true
 
   validates :personal_access_token_prefix,

app/models/application_setting_implementation.rb

@@ -60,6 +60,8 @@ module ApplicationSettingImplementation
         default_projects_limit: Settings.gitlab['default_projects_limit'],
         default_snippet_visibility: Settings.gitlab.default_projects_features['visibility_level'],
         diff_max_patch_bytes: Gitlab::Git::Diff::DEFAULT_MAX_PATCH_BYTES,
+        diff_max_files: Commit::DEFAULT_MAX_DIFF_FILES_SETTING,
+        diff_max_lines: Commit::DEFAULT_MAX_DIFF_LINES_SETTING,
         disable_feed_token: false,
         disabled_oauth_sign_in_sources: [],
         dns_rebinding_protection_enabled: true,

app/models/commit.rb

@@ -33,6 +33,12 @@ class Commit
   # Used by GFM to match and present link extensions on node texts and hrefs.
   LINK_EXTENSION_PATTERN = /(patch)/.freeze
 
+  DEFAULT_MAX_DIFF_LINES_SETTING = 50_000
+  DEFAULT_MAX_DIFF_FILES_SETTING = 1_000
+  MAX_DIFF_LINES_SETTING_UPPER_BOUND = 100_000
+  MAX_DIFF_FILES_SETTING_UPPER_BOUND = 3_000
+  DIFF_SAFE_LIMIT_FACTOR = 10
+
   cache_markdown_field :title, pipeline: :single_line
   cache_markdown_field :full_title, pipeline: :single_line, limit: 1.kilobyte
   cache_markdown_field :description, pipeline: :commit_description, limit: 1.megabyte
@@ -78,20 +84,24 @@ class Commit
     end
 
     def diff_safe_lines(project: nil)
-      Gitlab::Git::DiffCollection.default_limits(project: project)[:max_lines]
+      diff_safe_max_lines(project: project)
     end
 
-    def diff_hard_limit_files(project: nil)
+    def diff_max_files(project: nil)
       if Feature.enabled?(:increased_diff_limits, project)
         3000
+      elsif Feature.enabled?(:configurable_diff_limits, project)
+        Gitlab::CurrentSettings.diff_max_files
       else
         1000
       end
     end
 
-    def diff_hard_limit_lines(project: nil)
+    def diff_max_lines(project: nil)
       if Feature.enabled?(:increased_diff_limits, project)
         100000
+      elsif Feature.enabled?(:configurable_diff_limits, project)
+        Gitlab::CurrentSettings.diff_max_lines
       else
         50000
       end
@@ -99,11 +109,19 @@ class Commit
 
     def max_diff_options(project: nil)
       {
-        max_files: diff_hard_limit_files(project: project),
-        max_lines: diff_hard_limit_lines(project: project)
+        max_files: diff_max_files(project: project),
+        max_lines: diff_max_lines(project: project)
       }
     end
 
+    def diff_safe_max_files(project: nil)
+      diff_max_files(project: project) / DIFF_SAFE_LIMIT_FACTOR
+    end
+
+    def diff_safe_max_lines(project: nil)
+      diff_max_lines(project: project) / DIFF_SAFE_LIMIT_FACTOR
+    end
+
     def from_hash(hash, container)
       raw_commit = Gitlab::Git::Commit.new(container.repository.raw, hash)
       new(raw_commit, container)

app/views/admin/application_settings/_diff_limits.html.haml

@@ -3,11 +3,30 @@
 
   %fieldset
     .form-group
-      = f.label :diff_max_patch_bytes, _('Maximum diff patch size in bytes'), class: 'label-light'
+      = f.label :diff_max_patch_bytes, _('Maximum diff patch size (Bytes)'), class: 'label-light'
       = f.number_field :diff_max_patch_bytes, class: 'form-control gl-form-input'
       %span.form-text.text-muted
-        = _("Collapse diffs larger than this size, and show a 'too large' message instead.")
+        = _("Diff files surpassing this limit will be presented as 'too large' and won't be expandable.")
 
         = link_to sprite_icon('question-o'),
-          help_page_path('user/admin_area/diff_limits')
+          help_page_path('user/admin_area/diff_limits',
+          anchor: 'diff-limits-administration')
+
+      = f.label :diff_max_files, _('Maximum files in a diff'), class: 'label-light'
+      = f.number_field :diff_max_files, class: 'form-control gl-form-input'
+      %span.form-text.text-muted
+        = _("Diff files surpassing this limit will be presented as 'too large' and won't be expandable.")
+
+        = link_to sprite_icon('question-o'),
+          help_page_path('user/admin_area/diff_limits',
+          anchor: 'diff-limits-administration')
+
+      = f.label :diff_max_lines, _('Maximum lines in a diff'), class: 'label-light'
+      = f.number_field :diff_max_lines, class: 'form-control gl-form-input'
+      %span.form-text.text-muted
+        = _("Diff files surpassing this limit will be presented as 'too large' and won't be expandable.")
+
+        = link_to sprite_icon('question-o'),
+          help_page_path('user/admin_area/diff_limits',
+          anchor: 'diff-limits-administration')
   = f.submit _('Save changes'), class: 'gl-button btn btn-confirm'

config/feature_flags/development/configurable_diff_limits.yml

+---
+name: configurable_diff_limits
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/56722
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/332194
+milestone: '14.0'
+type: development
+group: group::code review
+default_enabled: false

db/migrate/20210527133919_add_diff_max_lines_to_application_settings.rb

+# frozen_string_literal: true
+
+class AddDiffMaxLinesToApplicationSettings < ActiveRecord::Migration[6.0]
+  def change
+    add_column(:application_settings,
+               :diff_max_lines,
+               :integer,
+               default: 50000,
+               null: false)
+  end
+end

db/migrate/20210527134019_add_diff_max_files_to_application_settings.rb

+# frozen_string_literal: true
+
+class AddDiffMaxFilesToApplicationSettings < ActiveRecord::Migration[6.0]
+  def change
+    add_column(:application_settings,
+               :diff_max_files,
+               :integer,
+               default: 1000,
+               null: false)
+  end
+end

db/schema_migrations/20210527133919

+aaf5936c945451fa98df7c21ab34c9aa7190dcf301f536c259e5b1fe54407f36
\ No newline at end of file

db/schema_migrations/20210527134019

+ac4522ee51d4a4cda317b680c16be3d9ef3e1619bba80c26aefe8d5dc70f013c
\ No newline at end of file

db/structure.sql

@@ -9517,6 +9517,8 @@ CREATE TABLE application_settings (
     elasticsearch_username text,
     encrypted_elasticsearch_password bytea,
     encrypted_elasticsearch_password_iv bytea,
+    diff_max_lines integer DEFAULT 50000 NOT NULL,
+    diff_max_files integer DEFAULT 1000 NOT NULL,
     CONSTRAINT app_settings_container_reg_cleanup_tags_max_list_size_positive CHECK ((container_registry_cleanup_tags_service_max_list_size >= 0)),
     CONSTRAINT app_settings_ext_pipeline_validation_service_url_text_limit CHECK ((char_length(external_pipeline_validation_service_url) <= 255)),
     CONSTRAINT app_settings_registry_exp_policies_worker_capacity_positive CHECK ((container_registry_expiration_policies_worker_capacity >= 0)),

doc/api/settings.md

@@ -251,7 +251,9 @@ listed in the descriptions of the relevant settings.
 | `default_projects_limit`                 | integer          | no                                   | Project limit per user. Default is `100000`. |
 | `default_snippet_visibility`             | string           | no                                   | What visibility level new snippets receive. Can take `private`, `internal` and `public` as a parameter. Default is `private`. |
 | `deletion_adjourned_period`              | integer          | no                                   | **(PREMIUM SELF)** The number of days to wait before deleting a project or group that is marked for deletion. Value must be between 0 and 90.
-| `diff_max_patch_bytes`                   | integer          | no                                   | Maximum diff patch size (Bytes). |
+| `diff_max_patch_bytes`                   | integer          | no                                   | Maximum [diff patch size](../user/admin_area/diff_limits.md), in bytes. |
+| `diff_max_files`                         | integer          | no                                   | Maximum [files in a diff](../user/admin_area/diff_limits.md). |
+| `diff_max_lines`                         | integer          | no                                   | Maximum [lines in a diff](../user/admin_area/diff_limits.md). |
 | `disable_feed_token`                     | boolean          | no                                   | Disable display of RSS/Atom and calendar feed tokens ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/231493) in GitLab 13.7) |
 | `disabled_oauth_sign_in_sources`         | array of strings | no                                   | Disabled OAuth sign-in sources. |
 | `dns_rebinding_protection_enabled`       | boolean          | no                                   | Enforce DNS rebinding attack protection. |

doc/user/admin_area/diff_limits.md

@@ -12,17 +12,25 @@ You can set a maximum size for display of diff files (patches).
 For details about diff files, [view changes between files](../project/merge_requests/changes.md).
 Read more about the [built-in limits for merge requests and diffs](../../administration/instance_limits.md#merge-requests).
 
-## Maximum diff patch size
+## Configure diff limits
 
-Diff files which exceed this value are presented as 'too large' and cannot
-be expandable. Instead of an expandable view, a link to the blob view is
-shown.
+WARNING:
+These settings are experimental. An increased maximum increases resource
+consumption of your instance. Keep this in mind when adjusting the maximum.
+
+To speed the loading time of merge request views and branch comparison views
+on your instance, you can configure three instance-level maximum values for diffs:
+
+- **Maximum diff patch size**: The total size, in bytes, of the entire diff.
+- **Maximum diff files**: The total number of files changed in a diff.
+- **Maximum diff files**: The total number of files changed in a diff. The default value is 1000.
+- **Maximum diff lines**: The total number of lines changed in a diff. The default value is 50,000.
 
-Patches greater than 10% of this size are automatically collapsed, and a
-link to expand the diff is presented.
-This affects merge requests and branch comparison views.
+When a diff reaches 10% of any of these values, the files are shown in a
+collapsed view, with a link to expand the diff. Diffs that exceed any of the
+set values are presented as **Too large** are cannot be expanded in the UI.
 
-To set the maximum diff patch size:
+To configure these values:
 
 1. On the top bar, select **Menu >** **{admin}** **Admin**.
 1. In the left sidebar, select **Settings > General**.
@@ -30,10 +38,6 @@ To set the maximum diff patch size:
 1. Enter a value for **Maximum diff patch size**, measured in bytes.
 1. Select **Save changes**.
 
-WARNING:
-This setting is experimental. An increased maximum increases resource
-consumption of your instance. Keep this in mind when adjusting the maximum.
-
 <!-- ## Troubleshooting
 
 Include any troubleshooting steps that you can foresee. If you know beforehand what issues

lib/gitlab/git/diff_collection.rb

@@ -12,11 +12,7 @@ module Gitlab
       delegate :max_files, :max_lines, :max_bytes, :safe_max_files, :safe_max_lines, :safe_max_bytes, to: :limits
 
       def self.default_limits(project: nil)
-        if Feature.enabled?(:increased_diff_limits, project)
-          { max_files: 300, max_lines: 10000 }
-        else
-          { max_files: 100, max_lines: 5000 }
-        end
+        { max_files: ::Commit.diff_safe_max_files(project: project), max_lines: ::Commit.diff_safe_max_lines(project: project) }
       end
 
       def self.limits(options = {})

locale/gitlab.pot

@@ -7969,9 +7969,6 @@ msgstr ""
 msgid "Collapse approvers"
 msgstr ""
 
-msgid "Collapse diffs larger than this size, and show a 'too large' message instead."
-msgstr ""
-
 msgid "Collapse issues"
 msgstr ""
 
@@ -11417,6 +11414,9 @@ msgstr ""
 msgid "Didn't receive unlock instructions?"
 msgstr ""
 
+msgid "Diff files surpassing this limit will be presented as 'too large' and won't be expandable."
+msgstr ""
+
 msgid "Diff limits"
 msgstr ""
 
@@ -20252,7 +20252,7 @@ msgstr ""
 msgid "Maximum delay (Minutes)"
 msgstr ""
 
-msgid "Maximum diff patch size in bytes"
+msgid "Maximum diff patch size (Bytes)"
 msgstr ""
 
 msgid "Maximum duration of a session."
@@ -20276,6 +20276,9 @@ msgstr ""
 msgid "Maximum file size is 2MB. Please select a smaller file."
 msgstr ""
 
+msgid "Maximum files in a diff"
+msgstr ""
+
 msgid "Maximum import size (MB)"
 msgstr ""
 
@@ -20288,6 +20291,9 @@ msgstr ""
 msgid "Maximum lifetime allowable for Personal Access Tokens is active, your expire date must be set before %{maximum_allowable_date}."
 msgstr ""
 
+msgid "Maximum lines in a diff"
+msgstr ""
+
 msgid "Maximum npm package file size in bytes"
 msgstr ""
 

spec/models/application_setting_spec.rb

@@ -990,6 +990,34 @@ RSpec.describe ApplicationSetting do
         end
       end
     end
+
+    describe '#diff_max_files' do
+      context 'validations' do
+        it { is_expected.to validate_presence_of(:diff_max_files) }
+
+        specify do
+          is_expected
+            .to validate_numericality_of(:diff_max_files)
+            .only_integer
+            .is_greater_than_or_equal_to(Commit::DEFAULT_MAX_DIFF_FILES_SETTING)
+            .is_less_than_or_equal_to(Commit::MAX_DIFF_FILES_SETTING_UPPER_BOUND)
+        end
+      end
+    end
+
+    describe '#diff_max_lines' do
+      context 'validations' do
+        it { is_expected.to validate_presence_of(:diff_max_lines) }
+
+        specify do
+          is_expected
+            .to validate_numericality_of(:diff_max_lines)
+            .only_integer
+            .is_greater_than_or_equal_to(Commit::DEFAULT_MAX_DIFF_LINES_SETTING)
+            .is_less_than_or_equal_to(Commit::MAX_DIFF_LINES_SETTING_UPPER_BOUND)
+        end
+      end
+    end
   end
 
   describe '#sourcegraph_url_is_com?' do

spec/models/commit_spec.rb

@@ -672,6 +672,92 @@ eos
     it_behaves_like '#uri_type'
   end
 
+  describe '.diff_max_files' do
+    subject(:diff_max_files) { described_class.diff_max_files }
+
+    let(:increased_diff_limits) { false }
+    let(:configurable_diff_limits) { false }
+
+    before do
+      stub_feature_flags(increased_diff_limits: increased_diff_limits, configurable_diff_limits: configurable_diff_limits)
+    end
+
+    context 'when increased_diff_limits is enabled' do
+      let(:increased_diff_limits) { true }
+
+      it 'returns 3000' do
+        expect(diff_max_files).to eq(3000)
+      end
+    end
+
+    context 'when configurable_diff_limits is enabled' do
+      let(:configurable_diff_limits) { true }
+
+      it 'returns the current settings' do
+        Gitlab::CurrentSettings.update!(diff_max_files: 1234)
+        expect(diff_max_files).to eq(1234)
+      end
+    end
+
+    context 'when neither feature flag is enabled' do
+      it 'returns 1000' do
+        expect(diff_max_files).to eq(1000)
+      end
+    end
+  end
+
+  describe '.diff_max_lines' do
+    subject(:diff_max_lines) { described_class.diff_max_lines }
+
+    let(:increased_diff_limits) { false }
+    let(:configurable_diff_limits) { false }
+
+    before do
+      stub_feature_flags(increased_diff_limits: increased_diff_limits, configurable_diff_limits: configurable_diff_limits)
+    end
+
+    context 'when increased_diff_limits is enabled' do
+      let(:increased_diff_limits) { true }
+
+      it 'returns 100000' do
+        expect(diff_max_lines).to eq(100000)
+      end
+    end
+
+    context 'when configurable_diff_limits is enabled' do
+      let(:configurable_diff_limits) { true }
+
+      it 'returns the current settings' do
+        Gitlab::CurrentSettings.update!(diff_max_lines: 65321)
+        expect(diff_max_lines).to eq(65321)
+      end
+    end
+
+    context 'when neither feature flag is enabled' do
+      it 'returns 50000' do
+        expect(diff_max_lines).to eq(50000)
+      end
+    end
+  end
+
+  describe '.diff_safe_max_files' do
+    subject(:diff_safe_max_files) { described_class.diff_safe_max_files }
+
+    it 'returns the commit diff max divided by the limit factor of 10' do
+      expect(::Commit).to receive(:diff_max_files).and_return(10)
+      expect(diff_safe_max_files).to eq(1)
+    end
+  end
+
+  describe '.diff_safe_max_lines' do
+    subject(:diff_safe_max_lines) { described_class.diff_safe_max_lines }
+
+    it 'returns the commit diff max divided by the limit factor of 10' do
+      expect(::Commit).to receive(:diff_max_lines).and_return(100)
+      expect(diff_safe_max_lines).to eq(10)
+    end
+  end
+
   describe '.from_hash' do
     subject { described_class.from_hash(commit.to_hash, container) }
 

spec/requests/api/merge_requests_spec.rb

@@ -1353,7 +1353,7 @@ RSpec.describe API::MergeRequests do
 
     context 'when a merge request has more than the changes limit' do
       it "returns a string indicating that more changes were made" do
-        allow(Commit).to receive(:diff_hard_limit_files).and_return(5)
+        allow(Commit).to receive(:diff_max_files).and_return(5)
 
         merge_request_overflow = create(:merge_request, :simple,
                                         author: user,

spec/requests/api/settings_spec.rb

@@ -113,6 +113,8 @@ RSpec.describe API::Settings, 'Settings', :do_not_mock_admin_mode_setting do
             terms: 'Hello world!',
             performance_bar_allowed_group_path: group.full_path,
             diff_max_patch_bytes: 300_000,
+            diff_max_files: 2000,
+            diff_max_lines: 50000,
             default_branch_protection: ::Gitlab::Access::PROTECTION_DEV_CAN_MERGE,
             local_markdown_version: 3,
             allow_local_requests_from_web_hooks_and_services: true,
@@ -159,6 +161,8 @@ RSpec.describe API::Settings, 'Settings', :do_not_mock_admin_mode_setting do
         expect(json_response['terms']).to eq('Hello world!')
         expect(json_response['performance_bar_allowed_group_id']).to eq(group.id)
         expect(json_response['diff_max_patch_bytes']).to eq(300_000)
+        expect(json_response['diff_max_files']).to eq(2000)
+        expect(json_response['diff_max_lines']).to eq(50000)
         expect(json_response['default_branch_protection']).to eq(Gitlab::Access::PROTECTION_DEV_CAN_MERGE)
         expect(json_response['local_markdown_version']).to eq(3)
         expect(json_response['allow_local_requests_from_web_hooks_and_services']).to eq(true)