diff options
author | Oswaldo Ferreira <oswaldo@gitlab.com> | 2018-09-24 18:30:49 +0300 |
---|---|---|
committer | Oswaldo Ferreira <oswaldo@gitlab.com> | 2018-10-01 17:58:21 +0300 |
commit | 4fbca2a346dc4c2c2c57e6a5bc3d13a8c3eeb23e (patch) | |
tree | 85ac3e8ffe36470fa4fa424c58e372d585417f03 | |
parent | 4d4522c15860529693305dc0fc0231c22eaf8a31 (diff) |
Make single diff patch limit configurable
- Creates a new column to hold the single patch limit value on
application_settings
- Allows updating this value through the application_settings API
- Calculates single diff patch collapsing limit based on
diff_max_patch_bytes column
- Updates diff limit documentation
- Adds documentation (with warning) as of how one can update this limit
-rw-r--r-- | app/helpers/application_settings_helper.rb | 3 | ||||
-rw-r--r-- | app/models/application_setting.rb | 9 | ||||
-rw-r--r-- | app/views/admin/application_settings/_diff_limits.html.haml | 16 | ||||
-rw-r--r-- | app/views/admin/application_settings/show.html.haml | 11 | ||||
-rw-r--r-- | changelogs/unreleased/osw-configurable-single-diff-file-limit.yml | 5 | ||||
-rw-r--r-- | db/migrate/20180924141949_add_diff_max_patch_bytes_to_application_settings.rb | 25 | ||||
-rw-r--r-- | db/schema.rb | 3 | ||||
-rw-r--r-- | doc/administration/index.md | 1 | ||||
-rw-r--r-- | doc/development/diffs.md | 27 | ||||
-rw-r--r-- | doc/user/admin_area/diff_limits.md | 21 | ||||
-rw-r--r-- | lib/gitlab/git/diff.rb | 62 | ||||
-rw-r--r-- | lib/gitlab/git/diff_collection.rb | 2 | ||||
-rw-r--r-- | locale/gitlab.pot | 6 | ||||
-rw-r--r-- | spec/models/application_setting_spec.rb | 15 | ||||
-rw-r--r-- | spec/requests/api/settings_spec.rb | 4 |
15 files changed, 158 insertions, 52 deletions
diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index c9a5431d18e..15cbfeea609 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -254,7 +254,8 @@ module ApplicationSettingsHelper :user_default_internal_regex, :user_oauth_applications, :version_check_enabled, - :web_ide_clientside_preview_enabled + :web_ide_clientside_preview_enabled, + :diff_max_patch_bytes ] end diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 5f835a8da75..65a2f760f93 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -182,6 +182,12 @@ class ApplicationSetting < ActiveRecord::Base numericality: { less_than_or_equal_to: :gitaly_timeout_default }, if: :gitaly_timeout_default + validates :diff_max_patch_bytes, + presence: true, + numericality: { only_integer: true, + 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 :user_default_internal_regex, js_regex: true, allow_nil: true SUPPORTED_KEY_TYPES.each do |type| @@ -293,7 +299,8 @@ class ApplicationSetting < ActiveRecord::Base user_default_external: false, user_default_internal_regex: nil, user_show_add_ssh_key_message: true, - usage_stats_set_by_user_id: nil + usage_stats_set_by_user_id: nil, + diff_max_patch_bytes: Gitlab::Git::Diff::DEFAULT_MAX_PATCH_BYTES } end diff --git a/app/views/admin/application_settings/_diff_limits.html.haml b/app/views/admin/application_settings/_diff_limits.html.haml new file mode 100644 index 00000000000..408e569fe07 --- /dev/null +++ b/app/views/admin/application_settings/_diff_limits.html.haml @@ -0,0 +1,16 @@ += form_for @application_setting, url: admin_application_settings_path(anchor: 'js-merge-request-settings'), html: { class: 'fieldset-form' } do |f| + = form_errors(@application_setting) + + %fieldset + .form-group + = f.label :diff_max_patch_bytes, 'Maximum diff patch size (Bytes)', class: 'label-light' + = f.number_field :diff_max_patch_bytes, class: 'form-control' + %span.form-text.text-muted + Diff files surpassing this limit will be presented as 'too large' + and won't be expandable. + + = link_to icon('question-circle'), + help_page_path('user/admin_area/diff_limits', + anchor: 'maximum-diff-patch-size') + + = f.submit _('Save changes'), class: 'btn btn-success' diff --git a/app/views/admin/application_settings/show.html.haml b/app/views/admin/application_settings/show.html.haml index e2043183a97..761555c4189 100644 --- a/app/views/admin/application_settings/show.html.haml +++ b/app/views/admin/application_settings/show.html.haml @@ -24,6 +24,17 @@ .settings-content = render 'account_and_limit' +%section.settings.as-diff-limits.no-animate#js-merge-request-settings{ class: ('expanded' if expanded_by_default?) } + .settings-header + %h4 + = _('Diff limits') + %button.btn.js-settings-toggle{ type: 'button' } + = expanded_by_default? ? _('Collapse') : _('Expand') + %p + = _('Diff content limits') + .settings-content + = render 'diff_limits' + %section.settings.as-signup.no-animate#js-signup-settings{ class: ('expanded' if expanded_by_default?) } .settings-header %h4 diff --git a/changelogs/unreleased/osw-configurable-single-diff-file-limit.yml b/changelogs/unreleased/osw-configurable-single-diff-file-limit.yml new file mode 100644 index 00000000000..55a4b305885 --- /dev/null +++ b/changelogs/unreleased/osw-configurable-single-diff-file-limit.yml @@ -0,0 +1,5 @@ +--- +title: Make single diff patch limit configurable +merge_request: 21886 +author: +type: added diff --git a/db/migrate/20180924141949_add_diff_max_patch_bytes_to_application_settings.rb b/db/migrate/20180924141949_add_diff_max_patch_bytes_to_application_settings.rb new file mode 100644 index 00000000000..084dfc65ce5 --- /dev/null +++ b/db/migrate/20180924141949_add_diff_max_patch_bytes_to_application_settings.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddDiffMaxPatchBytesToApplicationSettings < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_column_with_default(:application_settings, + :diff_max_patch_bytes, + :integer, + default: 100.kilobytes, + allow_null: false) + end + + def down + remove_column(:application_settings, :diff_max_patch_bytes) + end +end diff --git a/db/schema.rb b/db/schema.rb index f92d8005dfb..2e0152af81d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20180917172041) do +ActiveRecord::Schema.define(version: 20180924141949) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -171,6 +171,7 @@ ActiveRecord::Schema.define(version: 20180917172041) do t.boolean "user_show_add_ssh_key_message", default: true, null: false t.integer "usage_stats_set_by_user_id" t.integer "receive_max_input_size" + t.integer "diff_max_patch_bytes", default: 102400, null: false end create_table "audit_events", force: :cascade do |t| diff --git a/doc/administration/index.md b/doc/administration/index.md index 702d8e554a8..d713247983b 100644 --- a/doc/administration/index.md +++ b/doc/administration/index.md @@ -47,6 +47,7 @@ Learn how to install, configure, update, and maintain your GitLab instance. - [Enforcing Terms of Service](../user/admin_area/settings/terms.md) - [Third party offers](../user/admin_area/settings/third_party_offers.md) - [Compliance](compliance.md): A collection of features from across the application that you may configure to help ensure that your GitLab instance and DevOps workflow meet compliance standards. +- [Diff limits](../user/admin_area/diff_limits.md): Configure the diff rendering size limits of branch comparison pages. #### Customizing GitLab's appearance diff --git a/doc/development/diffs.md b/doc/development/diffs.md index 5e8e8cc7541..4adae5dabe2 100644 --- a/doc/development/diffs.md +++ b/doc/development/diffs.md @@ -2,13 +2,10 @@ Currently we rely on different sources to present diffs, these include: -- Rugged gem - Gitaly service - Database (through `merge_request_diff_files`) - Redis (cached highlighted diffs) -We're constantly moving Rugged calls to Gitaly and the progress can be followed through [Gitaly repo](https://gitlab.com/gitlab-org/gitaly). - ## Architecture overview ### Merge request diffs @@ -19,8 +16,9 @@ we fetch the comparison information using `Gitlab::Git::Compare`, which fetches The diffs fetching process _limits_ single file diff sizes and the overall size of the whole diff through a series of constant values. Raw diff files are then persisted on `merge_request_diff_files` table. -Even though diffs higher than 10kb are collapsed (`Gitlab::Git::Diff::COLLAPSE_LIMIT`), we still keep them on Postgres. However, diff files over _safety limits_ -(see the [Diff limits section](#diff-limits)) are _not_ persisted. +Even though diffs larger than 10% of the value of `ApplicationSettings#diff_max_patch_bytes` are collapsed, +we still keep them on Postgres. However, diff files larger than defined _safety limits_ +(see the [Diff limits section](#diff-limits)) are _not_ persisted in the database. In order to present diffs information on the Merge Request diffs page, we: @@ -102,23 +100,20 @@ Gitaly will only return the safe amount of data to be persisted on `merge_reques Limits that act onto each diff file of a collection. Files number, lines number and files size are considered. -```ruby -Gitlab::Git::Diff::COLLAPSE_LIMIT = 10.kilobytes -``` +#### Expandable patches (collapsed) -File diff will be collapsed (but be expandable) if it is larger than 10 kilobytes. +Diff patches are collapsed when surpassing 10% of the value set in `ApplicationSettings#diff_max_patch_bytes`. +That is, it's equivalent to 10kb if the maximum allowed value is 100kb. +The diff will still be persisted and expandable if the patch size doesn't +surpass `ApplicationSettings#diff_max_patch_bytes`. *Note:* Although this nomenclature (Collapsing) is also used on Gitaly, this limit is only used on GitLab (hardcoded - not sent to Gitaly). Gitaly will only return `Diff.Collapsed` (RPC) when surpassing collection limits. -```ruby -Gitlab::Git::Diff::SIZE_LIMIT = 100.kilobytes -``` - -File diff will not be rendered if it's larger than 100 kilobytes. +#### Not expandable patches (too large) -*Note:* This limit is currently hardcoded and applied on Gitaly and the RPC returns `Diff.TooLarge` when this limit is surpassed. -Although we're still also applying it on GitLab, we should remove the redundancy from GitLab once we're confident with the Gitaly integration. +The patch not be rendered if it's larger than `ApplicationSettings#diff_max_patch_bytes`. +Users will see a `This source diff could not be displayed because it is too large` message. ```ruby Commit::DIFF_SAFE_LINES = Gitlab::Git::DiffCollection::DEFAULT_LIMITS[:max_lines] = 5000 diff --git a/doc/user/admin_area/diff_limits.md b/doc/user/admin_area/diff_limits.md new file mode 100644 index 00000000000..9205860ef1f --- /dev/null +++ b/doc/user/admin_area/diff_limits.md @@ -0,0 +1,21 @@ +# Diff limits administration + +NOTE: **Note:** +Merge requests and branch comparison views will be affected. + +CAUTION: **Caution:** +These settings are currently under experimental state. They'll +increase the resource consumption of your instance and should +be edited mindfully. + +1. Access **Admin area > Settings > General** +1. Expand **Diff limits** + +### Maximum diff patch size + +This is the content size each diff file (patch) is allowed to reach before +it's collapsed, without the possibility of being expanded. A link redirecting +to the blob view will be presented for the patches that surpass this limit. + +Patches surpassing 10% of this content size will be automatically collapsed, +but expandable (a link to expand the diff will be presented). diff --git a/lib/gitlab/git/diff.rb b/lib/gitlab/git/diff.rb index f6b51dc3982..0d96211f4d4 100644 --- a/lib/gitlab/git/diff.rb +++ b/lib/gitlab/git/diff.rb @@ -19,13 +19,17 @@ module Gitlab alias_method :expanded?, :expanded - SERIALIZE_KEYS = %i(diff new_path old_path a_mode b_mode new_file renamed_file deleted_file too_large).freeze + # The default maximum content size to display a diff patch. + # + # If this value ever changes, make sure to create a migration to update + # current records, and default of `ApplicationSettings#diff_max_patch_bytes`. + DEFAULT_MAX_PATCH_BYTES = 100.kilobytes - # The maximum size of a diff to display. - SIZE_LIMIT = 100.kilobytes + # This is a limitation applied on the source (Gitaly), therefore we don't allow + # persisting limits over that. + MAX_PATCH_BYTES_UPPER_BOUND = 500.kilobytes - # The maximum size before a diff is collapsed. - COLLAPSE_LIMIT = 10.kilobytes + SERIALIZE_KEYS = %i(diff new_path old_path a_mode b_mode new_file renamed_file deleted_file too_large).freeze class << self def between(repo, head, base, options = {}, *paths) @@ -105,6 +109,26 @@ module Gitlab def binary_message(old_path, new_path) "Binary files #{old_path} and #{new_path} differ\n" end + + # Returns the limit of bytes a single diff file can reach before it + # appears as 'collapsed' for end-users. + # By convention, it's 10% of the persisted `diff_max_patch_bytes`. + # + # Example: If we have 100k for the `diff_max_patch_bytes`, it will be 10k by + # default. + # + # Patches surpassing this limit should still be persisted in the database. + def patch_safe_limit_bytes + patch_hard_limit_bytes / 10 + end + + # Returns the limit for a single diff file (patch). + # + # Patches surpassing this limit shouldn't be persisted in the database + # and will be presented as 'too large' for end-users. + def patch_hard_limit_bytes + Gitlab::CurrentSettings.diff_max_patch_bytes + end end def initialize(raw_diff, expanded: true) @@ -150,7 +174,7 @@ module Gitlab def too_large? if @too_large.nil? - @too_large = @diff.bytesize >= SIZE_LIMIT + @too_large = @diff.bytesize >= self.class.patch_hard_limit_bytes else @too_large end @@ -168,7 +192,7 @@ module Gitlab def collapsed? return @collapsed if defined?(@collapsed) - @collapsed = !expanded && @diff.bytesize >= COLLAPSE_LIMIT + @collapsed = !expanded && @diff.bytesize >= self.class.patch_safe_limit_bytes end def collapse! @@ -219,30 +243,6 @@ module Gitlab collapse! end end - - # If the patch surpasses any of the diff limits it calls the appropiate - # prune method and returns true. Otherwise returns false. - def prune_large_patch(patch) - size = 0 - - patch.each_hunk do |hunk| - hunk.each_line do |line| - size += line.content.bytesize - - if size >= SIZE_LIMIT - too_large! - return true # rubocop:disable Cop/AvoidReturnFromBlocks - end - end - end - - if !expanded && size >= COLLAPSE_LIMIT - collapse! - return true - end - - false - end end end end diff --git a/lib/gitlab/git/diff_collection.rb b/lib/gitlab/git/diff_collection.rb index 20dce8d0e06..47ebca7c4a2 100644 --- a/lib/gitlab/git/diff_collection.rb +++ b/lib/gitlab/git/diff_collection.rb @@ -19,7 +19,7 @@ module Gitlab limits[:safe_max_files] = [limits[:max_files], DEFAULT_LIMITS[:max_files]].min limits[:safe_max_lines] = [limits[:max_lines], DEFAULT_LIMITS[:max_lines]].min limits[:safe_max_bytes] = limits[:safe_max_files] * 5.kilobytes # Average 5 KB per file - limits[:max_patch_bytes] = Gitlab::Git::Diff::SIZE_LIMIT + limits[:max_patch_bytes] = Gitlab::Git::Diff.patch_hard_limit_bytes OpenStruct.new(limits) end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 1b66786a890..05e734b7375 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -2289,6 +2289,12 @@ msgstr "" msgid "Details" msgstr "" +msgid "Diff content limits" +msgstr "" + +msgid "Diff limits" +msgstr "" + msgid "Diffs|No file name available" msgstr "" diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index 9647c1b9f63..dcfc80daa57 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -592,4 +592,19 @@ describe ApplicationSetting do it { is_expected.to eq(result) } end end + + context 'diff limit settings' do + describe '#diff_max_patch_bytes' do + context 'validations' do + it { is_expected.to validate_presence_of(:diff_max_patch_bytes) } + + it do + is_expected.to validate_numericality_of(:diff_max_patch_bytes) + .only_integer + .is_greater_than_or_equal_to(Gitlab::Git::Diff::DEFAULT_MAX_PATCH_BYTES) + .is_less_than_or_equal_to(Gitlab::Git::Diff::MAX_PATCH_BYTES_UPPER_BOUND) + end + end + end + end end diff --git a/spec/requests/api/settings_spec.rb b/spec/requests/api/settings_spec.rb index 3e0f47b84a1..e379bd9785a 100644 --- a/spec/requests/api/settings_spec.rb +++ b/spec/requests/api/settings_spec.rb @@ -66,7 +66,8 @@ describe API::Settings, 'Settings' do enforce_terms: true, terms: 'Hello world!', performance_bar_allowed_group_path: group.full_path, - instance_statistics_visibility_private: true + instance_statistics_visibility_private: true, + diff_max_patch_bytes: 150_000 expect(response).to have_gitlab_http_status(200) expect(json_response['default_projects_limit']).to eq(3) @@ -92,6 +93,7 @@ describe API::Settings, 'Settings' do expect(json_response['terms']).to eq('Hello world!') expect(json_response['performance_bar_allowed_group_id']).to eq(group.id) expect(json_response['instance_statistics_visibility_private']).to be(true) + expect(json_response['diff_max_patch_bytes']).to eq(150_000) end end |