diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb
index 10c7b4032cf938d64b9f542007cc3e2283b57423..7bf3cb6230b7ea91539fdd45a7e823638b382f0d 100644
--- a/app/helpers/diff_helper.rb
+++ b/app/helpers/diff_helper.rb
@@ -190,12 +190,8 @@ def diff_file_changed_icon_color(diff_file)
def render_overflow_warning?(diffs_collection)
diff_files = diffs_collection.raw_diff_files
- if diff_files.any?(&:too_large?)
- Gitlab::Metrics.add_event(:diffs_overflow_single_file_limits)
- end
-
diff_files.overflow?.tap do |overflown|
- Gitlab::Metrics.add_event(:diffs_overflow_collection_limits) if overflown
+ log_overflow_limits(diff_files)
end
end
@@ -286,4 +282,18 @@ def conflicts
conflicts_service.conflicts.files.index_by(&:our_path)
end
+
+ def log_overflow_limits(diff_files)
+ if diff_files.any?(&:too_large?)
+ Gitlab::Metrics.add_event(:diffs_overflow_single_file_limits)
+ end
+
+ Gitlab::Metrics.add_event(:diffs_overflow_collection_limits) if diff_files.overflow?
+ Gitlab::Metrics.add_event(:diffs_overflow_max_bytes_limits) if diff_files.overflow_max_bytes?
+ Gitlab::Metrics.add_event(:diffs_overflow_max_files_limits) if diff_files.overflow_max_files?
+ Gitlab::Metrics.add_event(:diffs_overflow_max_lines_limits) if diff_files.overflow_max_lines?
+ Gitlab::Metrics.add_event(:diffs_overflow_collapsed_bytes_limits) if diff_files.collapsed_safe_bytes?
+ Gitlab::Metrics.add_event(:diffs_overflow_collapsed_files_limits) if diff_files.collapsed_safe_files?
+ Gitlab::Metrics.add_event(:diffs_overflow_collapsed_lines_limits) if diff_files.collapsed_safe_lines?
+ end
end
diff --git a/changelogs/unreleased/31063-ensure-diff-collection-limits-are-configurable.yml b/changelogs/unreleased/31063-ensure-diff-collection-limits-are-configurable.yml
new file mode 100644
index 0000000000000000000000000000000000000000..9e8918810c41d98544b2cfebd2d049d7e7b0fa36
--- /dev/null
+++ b/changelogs/unreleased/31063-ensure-diff-collection-limits-are-configurable.yml
@@ -0,0 +1,5 @@
+---
+title: Track the different overflows for diff collections
+merge_request: 57790
+author:
+type: other
diff --git a/lib/gitlab/git/diff_collection.rb b/lib/gitlab/git/diff_collection.rb
index 19462e6cb023d9dae211f57875bfa213fd3116c5..b7b6343aec041226d6b7b4297aeadaf26e072b1b 100644
--- a/lib/gitlab/git/diff_collection.rb
+++ b/lib/gitlab/git/diff_collection.rb
@@ -82,6 +82,30 @@ def overflow?
!!@overflow
end
+ def overflow_max_lines?
+ !!@overflow_max_lines
+ end
+
+ def overflow_max_bytes?
+ !!@overflow_max_bytes
+ end
+
+ def overflow_max_files?
+ !!@overflow_max_files
+ end
+
+ def collapsed_safe_lines?
+ !!@collapsed_safe_lines
+ end
+
+ def collapsed_safe_files?
+ !!@collapsed_safe_files
+ end
+
+ def collapsed_safe_bytes?
+ !!@collapsed_safe_bytes
+ end
+
def size
@size ||= count # forces a loop using each method
end
@@ -121,7 +145,15 @@ def populate!
end
def over_safe_limits?(files)
- files >= safe_max_files || @line_count > safe_max_lines || @byte_count >= safe_max_bytes
+ if files >= safe_max_files
+ @collapsed_safe_files = true
+ elsif @line_count > safe_max_lines
+ @collapsed_safe_lines = true
+ elsif @byte_count >= safe_max_bytes
+ @collapsed_safe_bytes = true
+ end
+
+ @collapsed_safe_files || @collapsed_safe_lines || @collapsed_safe_bytes
end
def expand_diff?
@@ -154,6 +186,7 @@ def each_serialized_patch
if @enforce_limits && i >= max_files
@overflow = true
+ @overflow_max_files = true
break
end
@@ -166,10 +199,19 @@ def each_serialized_patch
@line_count += diff.line_count
@byte_count += diff.diff.bytesize
- if @enforce_limits && (@line_count >= max_lines || @byte_count >= max_bytes)
+ if @enforce_limits && @line_count >= max_lines
+ # This last Diff instance pushes us over the lines limit. We stop and
+ # discard it.
+ @overflow = true
+ @overflow_max_lines = true
+ break
+ end
+
+ if @enforce_limits && @byte_count >= max_bytes
# This last Diff instance pushes us over the lines limit. We stop and
# discard it.
@overflow = true
+ @overflow_max_bytes = true
break
end
diff --git a/spec/helpers/diff_helper_spec.rb b/spec/helpers/diff_helper_spec.rb
index 20fa8d628841aa793b5e353fcafdc09c2ca62a2a..dfea1020c5294fbcf440423d5db4a0de8f11fd17 100644
--- a/spec/helpers/diff_helper_spec.rb
+++ b/spec/helpers/diff_helper_spec.rb
@@ -291,6 +291,8 @@
end
describe '#render_overflow_warning?' do
+ using RSpec::Parameterized::TableSyntax
+
let(:diffs_collection) { instance_double(Gitlab::Diff::FileCollection::MergeRequestDiff, raw_diff_files: diff_files) }
let(:diff_files) { Gitlab::Git::DiffCollection.new(files) }
let(:safe_file) { { too_large: false, diff: '' } }
@@ -299,13 +301,42 @@
before do
allow(diff_files).to receive(:overflow?).and_return(false)
+ allow(diff_files).to receive(:overflow_max_bytes?).and_return(false)
+ allow(diff_files).to receive(:overflow_max_files?).and_return(false)
+ allow(diff_files).to receive(:overflow_max_lines?).and_return(false)
+ allow(diff_files).to receive(:collapsed_safe_bytes?).and_return(false)
+ allow(diff_files).to receive(:collapsed_safe_files?).and_return(false)
+ allow(diff_files).to receive(:collapsed_safe_lines?).and_return(false)
end
- context 'when neither collection nor individual file hit the limit' do
+ context 'when no limits are hit' do
it 'returns false and does not log any overflow events' do
expect(Gitlab::Metrics).not_to receive(:add_event).with(:diffs_overflow_collection_limits)
expect(Gitlab::Metrics).not_to receive(:add_event).with(:diffs_overflow_single_file_limits)
+ expect(Gitlab::Metrics).not_to receive(:add_event).with(:diffs_overflow_max_bytes_limits)
+ expect(Gitlab::Metrics).not_to receive(:add_event).with(:diffs_overflow_max_files_limits)
+ expect(Gitlab::Metrics).not_to receive(:add_event).with(:diffs_overflow_max_lines_limits)
+ expect(Gitlab::Metrics).not_to receive(:add_event).with(:diffs_overflow_collapsed_bytes_limits)
+ expect(Gitlab::Metrics).not_to receive(:add_event).with(:diffs_overflow_collapsed_files_limits)
+ expect(Gitlab::Metrics).not_to receive(:add_event).with(:diffs_overflow_collapsed_lines_limits)
+
+ expect(render_overflow_warning?(diffs_collection)).to be false
+ end
+ end
+
+ where(:overflow_method, :event_name) do
+ :overflow_max_bytes? | :diffs_overflow_max_bytes_limits
+ :overflow_max_files? | :diffs_overflow_max_files_limits
+ :overflow_max_lines? | :diffs_overflow_max_lines_limits
+ :collapsed_safe_bytes? | :diffs_overflow_collapsed_bytes_limits
+ :collapsed_safe_files? | :diffs_overflow_collapsed_files_limits
+ :collapsed_safe_lines? | :diffs_overflow_collapsed_lines_limits
+ end
+ with_them do
+ it 'returns false and only logs the correct collection overflow event' do
+ allow(diff_files).to receive(overflow_method).and_return(true)
+ expect(Gitlab::Metrics).to receive(:add_event).with(event_name).once
expect(render_overflow_warning?(diffs_collection)).to be false
end
end
@@ -315,9 +346,8 @@
allow(diff_files).to receive(:overflow?).and_return(true)
end
- it 'returns false and only logs collection overflow event' do
- expect(Gitlab::Metrics).to receive(:add_event).with(:diffs_overflow_collection_limits).exactly(:once)
- expect(Gitlab::Metrics).not_to receive(:add_event).with(:diffs_overflow_single_file_limits)
+ it 'returns true and only logs all the correct collection overflow event' do
+ expect(Gitlab::Metrics).to receive(:add_event).with(:diffs_overflow_collection_limits).once
expect(render_overflow_warning?(diffs_collection)).to be true
end
diff --git a/spec/lib/gitlab/git/diff_collection_spec.rb b/spec/lib/gitlab/git/diff_collection_spec.rb
index 1a3c332a21bc5b89fb50ac5351e920027f7f60ea..114b3d01952e314c3c058aa8efb123e2a70c4cbb 100644
--- a/spec/lib/gitlab/git/diff_collection_spec.rb
+++ b/spec/lib/gitlab/git/diff_collection_spec.rb
@@ -31,6 +31,19 @@ def each
end
end
+ let(:overflow_max_bytes) { false }
+ let(:overflow_max_files) { false }
+ let(:overflow_max_lines) { false }
+
+ shared_examples 'overflow stuff' do
+ it 'returns the expected overflow values' do
+ subject.overflow?
+ expect(subject.overflow_max_bytes?).to eq(overflow_max_bytes)
+ expect(subject.overflow_max_files?).to eq(overflow_max_files)
+ expect(subject.overflow_max_lines?).to eq(overflow_max_lines)
+ end
+ end
+
subject do
Gitlab::Git::DiffCollection.new(
iterator,
@@ -76,12 +89,19 @@ def each
end
context 'overflow handling' do
+ subject { super() }
+
+ let(:collapsed_safe_files) { false }
+ let(:collapsed_safe_lines) { false }
+
context 'adding few enough files' do
let(:file_count) { 3 }
context 'and few enough lines' do
let(:line_count) { 10 }
+ it_behaves_like 'overflow stuff'
+
describe '#overflow?' do
subject { super().overflow? }
@@ -117,6 +137,11 @@ def each
context 'when limiting is disabled' do
let(:limits) { false }
+ let(:overflow_max_bytes) { false }
+ let(:overflow_max_files) { false }
+ let(:overflow_max_lines) { false }
+
+ it_behaves_like 'overflow stuff'
describe '#overflow?' do
subject { super().overflow? }
@@ -155,6 +180,9 @@ def each
context 'and too many lines' do
let(:line_count) { 1000 }
+ let(:overflow_max_lines) { true }
+
+ it_behaves_like 'overflow stuff'
describe '#overflow?' do
subject { super().overflow? }
@@ -184,6 +212,11 @@ def each
context 'when limiting is disabled' do
let(:limits) { false }
+ let(:overflow_max_bytes) { false }
+ let(:overflow_max_files) { false }
+ let(:overflow_max_lines) { false }
+
+ it_behaves_like 'overflow stuff'
describe '#overflow?' do
subject { super().overflow? }
@@ -216,10 +249,13 @@ def each
context 'adding too many files' do
let(:file_count) { 11 }
+ let(:overflow_max_files) { true }
context 'and few enough lines' do
let(:line_count) { 1 }
+ it_behaves_like 'overflow stuff'
+
describe '#overflow?' do
subject { super().overflow? }
@@ -248,6 +284,11 @@ def each
context 'when limiting is disabled' do
let(:limits) { false }
+ let(:overflow_max_bytes) { false }
+ let(:overflow_max_files) { false }
+ let(:overflow_max_lines) { false }
+
+ it_behaves_like 'overflow stuff'
describe '#overflow?' do
subject { super().overflow? }
@@ -279,6 +320,10 @@ def each
context 'and too many lines' do
let(:line_count) { 30 }
+ let(:overflow_max_lines) { true }
+ let(:overflow_max_files) { false }
+
+ it_behaves_like 'overflow stuff'
describe '#overflow?' do
subject { super().overflow? }
@@ -308,6 +353,11 @@ def each
context 'when limiting is disabled' do
let(:limits) { false }
+ let(:overflow_max_bytes) { false }
+ let(:overflow_max_files) { false }
+ let(:overflow_max_lines) { false }
+
+ it_behaves_like 'overflow stuff'
describe '#overflow?' do
subject { super().overflow? }
@@ -344,6 +394,8 @@ def each
context 'and few enough lines' do
let(:line_count) { 1 }
+ it_behaves_like 'overflow stuff'
+
describe '#overflow?' do
subject { super().overflow? }
@@ -375,6 +427,9 @@ def each
context 'adding too many bytes' do
let(:file_count) { 10 }
let(:line_length) { 5200 }
+ let(:overflow_max_bytes) { true }
+
+ it_behaves_like 'overflow stuff'
describe '#overflow?' do
subject { super().overflow? }
@@ -404,6 +459,11 @@ def each
context 'when limiting is disabled' do
let(:limits) { false }
+ let(:overflow_max_bytes) { false }
+ let(:overflow_max_files) { false }
+ let(:overflow_max_lines) { false }
+
+ it_behaves_like 'overflow stuff'
describe '#overflow?' do
subject { super().overflow? }
@@ -437,6 +497,8 @@ def each
describe 'empty collection' do
subject { Gitlab::Git::DiffCollection.new([]) }
+ it_behaves_like 'overflow stuff'
+
describe '#overflow?' do
subject { super().overflow? }
@@ -555,7 +617,7 @@ def each
.and_return({ max_files: 2, max_lines: max_lines })
end
- it 'prunes diffs by default even little ones' do
+ it 'prunes diffs by default even little ones and sets collapsed_safe_files true' do
subject.each_with_index do |d, i|
if i < 2
expect(d.diff).not_to eq('')
@@ -563,6 +625,8 @@ def each
expect(d.diff).to eq('')
end
end
+
+ expect(subject.collapsed_safe_files?).to eq(true)
end
end
@@ -582,7 +646,7 @@ def each
.and_return({ max_files: max_files, max_lines: 80 })
end
- it 'prunes diffs by default even little ones' do
+ it 'prunes diffs by default even little ones and sets collapsed_safe_lines true' do
subject.each_with_index do |d, i|
if i < 2
expect(d.diff).not_to eq('')
@@ -590,26 +654,30 @@ def each
expect(d.diff).to eq('')
end
end
+
+ expect(subject.collapsed_safe_lines?).to eq(true)
end
end
context 'when go over safe limits on bytes' do
let(:iterator) do
[
- fake_diff(1, 45),
- fake_diff(1, 45),
- fake_diff(1, 20480),
- fake_diff(1, 1)
+ fake_diff(5, 10),
+ fake_diff(5000, 10),
+ fake_diff(5, 10),
+ fake_diff(5, 10)
]
end
before do
+ allow(Gitlab::CurrentSettings).to receive(:diff_max_patch_bytes).and_return(1.megabyte)
+
allow(Gitlab::Git::DiffCollection)
.to receive(:default_limits)
- .and_return({ max_files: max_files, max_lines: 80 })
+ .and_return({ max_files: 4, max_lines: 3000 })
end
- it 'prunes diffs by default even little ones' do
+ it 'prunes diffs by default even little ones and sets collapsed_safe_bytes true' do
subject.each_with_index do |d, i|
if i < 2
expect(d.diff).not_to eq('')
@@ -617,6 +685,8 @@ def each
expect(d.diff).to eq('')
end
end
+
+ expect(subject.collapsed_safe_bytes?).to eq(true)
end
end
end