From 146baa597a9c73386551f618195c3c33d2322090 Mon Sep 17 00:00:00 2001 From: Emma Park Date: Tue, 12 Nov 2024 16:06:20 +0100 Subject: [PATCH 1/8] Add a new endpoint for compare It creates a new streaming endpoint for Compare to improve the performance of loading diff results. It renders diffs in HTML. MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/172404 Changlog: changed --- .../compare_diffs_stream_controller.rb | 21 +++++++++++++++++++ config/routes/repository.rb | 1 + 2 files changed, 22 insertions(+) create mode 100644 app/controllers/projects/compare_diffs_stream_controller.rb diff --git a/app/controllers/projects/compare_diffs_stream_controller.rb b/app/controllers/projects/compare_diffs_stream_controller.rb new file mode 100644 index 00000000000000..06119553884185 --- /dev/null +++ b/app/controllers/projects/compare_diffs_stream_controller.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module Projects + class CompareDiffsStreamController < Projects::CompareController + include StreamDiffs + + private + + def resource + compare + end + + def options + opts = diff_options + opts[:offset_index] = params.permit(:offset)[:offset].to_i + opts[:ignore_whitespace_change] = true if params.permit(:format)[:format] == 'diff' + opts[:use_extra_viewer_as_main] = false + opts + end + end +end diff --git a/config/routes/repository.rb b/config/routes/repository.rb index 2efea11c268a79..a8808f69340cfe 100644 --- a/config/routes/repository.rb +++ b/config/routes/repository.rb @@ -18,6 +18,7 @@ collection do get :diff_for_path get :signatures + get :diffs_stream, to: 'compare_diffs_stream#diffs' end end -- GitLab From f07aec55aa6f815ff7e9e8657bedebc6bc2c6ff5 Mon Sep 17 00:00:00 2001 From: Emma Park Date: Wed, 13 Nov 2024 17:49:46 +0100 Subject: [PATCH 2/8] Add some tests --- .../compare_diffs_stream_controller.rb | 8 --- app/models/compare.rb | 2 + .../compare_diffs_stream_controller_spec.rb | 71 +++++++++++++++++++ 3 files changed, 73 insertions(+), 8 deletions(-) create mode 100644 spec/requests/projects/compare_diffs_stream_controller_spec.rb diff --git a/app/controllers/projects/compare_diffs_stream_controller.rb b/app/controllers/projects/compare_diffs_stream_controller.rb index 06119553884185..1930400724bf20 100644 --- a/app/controllers/projects/compare_diffs_stream_controller.rb +++ b/app/controllers/projects/compare_diffs_stream_controller.rb @@ -9,13 +9,5 @@ class CompareDiffsStreamController < Projects::CompareController def resource compare end - - def options - opts = diff_options - opts[:offset_index] = params.permit(:offset)[:offset].to_i - opts[:ignore_whitespace_change] = true if params.permit(:format)[:format] == 'diff' - opts[:use_extra_viewer_as_main] = false - opts - end end end diff --git a/app/models/compare.rb b/app/models/compare.rb index 350149c08bf1d0..a4523417fd930f 100644 --- a/app/models/compare.rb +++ b/app/models/compare.rb @@ -90,6 +90,8 @@ def diffs(diff_options = nil) diff_refs: diff_refs) end + alias_method :diffs_for_streaming, :diffs + def diff_refs Gitlab::Diff::DiffRefs.new( base_sha: @straight ? start_commit_sha : base_commit_sha, diff --git a/spec/requests/projects/compare_diffs_stream_controller_spec.rb b/spec/requests/projects/compare_diffs_stream_controller_spec.rb new file mode 100644 index 00000000000000..185193d85d786e --- /dev/null +++ b/spec/requests/projects/compare_diffs_stream_controller_spec.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Compare diffs stream', feature_category: :source_code_management do + let_it_be(:project) { create(:project, :public, :repository) } + let_it_be(:user) { create(:user, maintainer_of: project) } + + let(:whitespace) { nil } + let(:straight) { nil } + let(:page) { nil } + let(:from_project_id) { nil } + let(:offset) { 0 } + let(:start_ref) { '08f22f25' } + let(:target_ref) { '66eceea0' } + + let(:compare) do + build(:compare, start_project: project, target_project: project, start_ref: start_ref, target_ref: target_ref, + straight: straight) + end + + let(:request_params) do + { + namespace_id: project.namespace, + project_id: project, + from_project_id: from_project_id, + from: start_ref, + to: target_ref, + w: whitespace, + page: page, + straight: straight + } + end + + describe 'GET diffs_stream' do + def send_request(**extra_params) + get diffs_stream_namespace_project_compare_index_path(request_params.merge(extra_params)) + end + + it 'streams the response' do + send_request + + expect(response).to have_gitlab_http_status(:success) + end + + it 'includes all diffs' do + send_request + + streamed_content = response.body + + compare.diffs.diff_files.each do |diff_file| + expect(streamed_content).to include(diff_file.new_path) + end + end + + context 'when offset is given' do + context 'when offset is 1' do + let(:offset) { 1 } + + it 'streams diffs except the offset' do + send_request + + diff_files = compare.diffs.diff_files.to_a + + expect(response.body).not_to include(diff_files.first.new_path) + expect(response.body).to include(diff_files.last.new_path) + end + end + end + end +end -- GitLab From 09560fc110a30f2006dac24f201947a892d3965f Mon Sep 17 00:00:00 2001 From: Emma Park Date: Thu, 14 Nov 2024 08:20:16 +0100 Subject: [PATCH 3/8] Remove a factory compare and use Compare model to fix tests --- .../compare_diffs_stream_controller_spec.rb | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/spec/requests/projects/compare_diffs_stream_controller_spec.rb b/spec/requests/projects/compare_diffs_stream_controller_spec.rb index 185193d85d786e..889c9c66699423 100644 --- a/spec/requests/projects/compare_diffs_stream_controller_spec.rb +++ b/spec/requests/projects/compare_diffs_stream_controller_spec.rb @@ -7,18 +7,13 @@ let_it_be(:user) { create(:user, maintainer_of: project) } let(:whitespace) { nil } - let(:straight) { nil } + let(:straight) { true } let(:page) { nil } let(:from_project_id) { nil } let(:offset) { 0 } let(:start_ref) { '08f22f25' } let(:target_ref) { '66eceea0' } - let(:compare) do - build(:compare, start_project: project, target_project: project, start_ref: start_ref, target_ref: target_ref, - straight: straight) - end - let(:request_params) do { namespace_id: project.namespace, @@ -32,6 +27,12 @@ } end + let(:raw_compare) do + project.repository.compare_source_branch(target_ref, project.repository, start_ref, straight: straight) + end + + let(:compare) { Compare.new(raw_compare, project, base_sha: nil, straight: straight) } + describe 'GET diffs_stream' do def send_request(**extra_params) get diffs_stream_namespace_project_compare_index_path(request_params.merge(extra_params)) -- GitLab From 44e8709bb4ae5f64b23186e339eece8308a04d02 Mon Sep 17 00:00:00 2001 From: Emma Park Date: Thu, 14 Nov 2024 09:09:34 +0100 Subject: [PATCH 4/8] Add more tests --- .../compare_diffs_stream_controller_spec.rb | 38 ++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/spec/requests/projects/compare_diffs_stream_controller_spec.rb b/spec/requests/projects/compare_diffs_stream_controller_spec.rb index 889c9c66699423..c8a0b9c29dc2fe 100644 --- a/spec/requests/projects/compare_diffs_stream_controller_spec.rb +++ b/spec/requests/projects/compare_diffs_stream_controller_spec.rb @@ -23,7 +23,8 @@ to: target_ref, w: whitespace, page: page, - straight: straight + straight: straight, + offset: offset } end @@ -67,6 +68,41 @@ def send_request(**extra_params) expect(response.body).to include(diff_files.last.new_path) end end + + context 'when offset is same as number of diffs' do + let(:offset) { compare.diffs.size } + + it 'no diffs are streamed' do + send_request + + expect(response.body).to be_empty + end + end + end + + context 'when an exception occurs' do + before do + allow(::RapidDiffs::DiffFileComponent) + .to receive(:new).and_raise(StandardError.new('something went wrong')) + end + + it 'prints out error message' do + send_request + + expect(response.body).to include('something went wrong') + end + end + + context 'when the rapid_diffs feature flag is disabled' do + before do + stub_feature_flags(rapid_diffs: false) + end + + it 'returns a 404 status' do + send_request + + expect(response).to have_gitlab_http_status(:not_found) + end end end end -- GitLab From 5f4324a1b7c47cb14dd5fbdb7ff745d821b4a8d0 Mon Sep 17 00:00:00 2001 From: Emma Park Date: Thu, 14 Nov 2024 14:30:01 +0100 Subject: [PATCH 5/8] Add offset_index param --- lib/gitlab/git/diff.rb | 2 +- spec/requests/projects/compare_diffs_stream_controller_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/git/diff.rb b/lib/gitlab/git/diff.rb index d09252ff033b92..2323b489e4d3a2 100644 --- a/lib/gitlab/git/diff.rb +++ b/lib/gitlab/git/diff.rb @@ -97,7 +97,7 @@ def between(repo, head, base, options = {}, *paths) # as generated. def filter_diff_options(options, default_options = {}) allowed_options = [:ignore_whitespace_change, :max_files, :max_lines, - :limits, :expanded, :collect_all_paths, :generated_files] + :limits, :expanded, :collect_all_paths, :generated_files, :offset_index] if default_options actual_defaults = default_options.dup diff --git a/spec/requests/projects/compare_diffs_stream_controller_spec.rb b/spec/requests/projects/compare_diffs_stream_controller_spec.rb index c8a0b9c29dc2fe..dc0a8a26edcd28 100644 --- a/spec/requests/projects/compare_diffs_stream_controller_spec.rb +++ b/spec/requests/projects/compare_diffs_stream_controller_spec.rb @@ -12,7 +12,7 @@ let(:from_project_id) { nil } let(:offset) { 0 } let(:start_ref) { '08f22f25' } - let(:target_ref) { '66eceea0' } + let(:target_ref) { 'master' } let(:request_params) do { -- GitLab From 5da84b047af7fea5d4cd8764da774a90150dd243 Mon Sep 17 00:00:00 2001 From: Emma Park Date: Thu, 14 Nov 2024 15:08:20 +0100 Subject: [PATCH 6/8] Fix failed test --- spec/requests/projects/compare_diffs_stream_controller_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/requests/projects/compare_diffs_stream_controller_spec.rb b/spec/requests/projects/compare_diffs_stream_controller_spec.rb index dc0a8a26edcd28..89e735b44607b0 100644 --- a/spec/requests/projects/compare_diffs_stream_controller_spec.rb +++ b/spec/requests/projects/compare_diffs_stream_controller_spec.rb @@ -51,7 +51,7 @@ def send_request(**extra_params) streamed_content = response.body compare.diffs.diff_files.each do |diff_file| - expect(streamed_content).to include(diff_file.new_path) + expect(streamed_content).to include(diff_file.old_path) end end -- GitLab From f1baf902491ed8a5c1b6a6951abad6014ed42f2f Mon Sep 17 00:00:00 2001 From: Emma Park Date: Fri, 15 Nov 2024 00:43:11 +0100 Subject: [PATCH 7/8] Add shared tests --- .../commit_diffs_stream_controller_spec.rb | 56 +----------------- .../compare_diffs_stream_controller_spec.rb | 59 +------------------ .../diffs_stream_shared_examples.rb | 58 ++++++++++++++++++ 3 files changed, 63 insertions(+), 110 deletions(-) create mode 100644 spec/support/shared_examples/diffs_stream_shared_examples.rb diff --git a/spec/requests/projects/commit_diffs_stream_controller_spec.rb b/spec/requests/projects/commit_diffs_stream_controller_spec.rb index d32c4f6c1b0e8b..debd71b6841c28 100644 --- a/spec/requests/projects/commit_diffs_stream_controller_spec.rb +++ b/spec/requests/projects/commit_diffs_stream_controller_spec.rb @@ -7,6 +7,7 @@ let_it_be(:user) { create(:user, maintainer_of: project) } let(:commit_with_two_diffs) { project.commit("874797c3a73b60d2187ed6e2fcabd289ff75171e") } let(:offset) { 0 } + let(:diff_files) { commit_with_two_diffs.diffs.diff_files } before do sign_in(user) @@ -24,12 +25,6 @@ def send_request(**extra_params) get diffs_stream_namespace_project_commit_path(params.merge(extra_params)) end - it 'streams the response' do - send_request - - expect(response).to have_gitlab_http_status(:success) - end - it 'includes all diffs' do send_request @@ -40,53 +35,6 @@ def send_request(**extra_params) end end - context 'when offset is given' do - context 'when offset is 1' do - let(:offset) { 1 } - - it 'streams diffs except the offset' do - send_request - - diff_files = commit_with_two_diffs.diffs.diff_files.to_a - expect(response.body).not_to include(diff_files.first.new_path) - expect(response.body).to include(diff_files.last.new_path) - end - end - - context 'when offset is same as number of diffs' do - let(:offset) { commit_with_two_diffs.diffs.size } - - it 'no diffs are streamed' do - send_request - - expect(response.body).to be_empty - end - end - end - - context 'when an exception occurs' do - before do - allow(::RapidDiffs::DiffFileComponent) - .to receive(:new).and_raise(StandardError.new('something went wrong')) - end - - it 'prints out error message' do - send_request - - expect(response.body).to include('something went wrong') - end - end - - context 'when the rapid_diffs feature flag is disabled' do - before do - stub_feature_flags(rapid_diffs: false) - end - - it 'returns a 404 status' do - send_request - - expect(response).to have_gitlab_http_status(:not_found) - end - end + include_examples 'diffs stream tests' end end diff --git a/spec/requests/projects/compare_diffs_stream_controller_spec.rb b/spec/requests/projects/compare_diffs_stream_controller_spec.rb index 89e735b44607b0..ae61785756b353 100644 --- a/spec/requests/projects/compare_diffs_stream_controller_spec.rb +++ b/spec/requests/projects/compare_diffs_stream_controller_spec.rb @@ -33,76 +33,23 @@ end let(:compare) { Compare.new(raw_compare, project, base_sha: nil, straight: straight) } + let(:diff_files) { compare.diffs.diff_files } describe 'GET diffs_stream' do def send_request(**extra_params) get diffs_stream_namespace_project_compare_index_path(request_params.merge(extra_params)) end - it 'streams the response' do - send_request - - expect(response).to have_gitlab_http_status(:success) - end - it 'includes all diffs' do send_request streamed_content = response.body - compare.diffs.diff_files.each do |diff_file| + diff_files.each do |diff_file| expect(streamed_content).to include(diff_file.old_path) end end - context 'when offset is given' do - context 'when offset is 1' do - let(:offset) { 1 } - - it 'streams diffs except the offset' do - send_request - - diff_files = compare.diffs.diff_files.to_a - - expect(response.body).not_to include(diff_files.first.new_path) - expect(response.body).to include(diff_files.last.new_path) - end - end - - context 'when offset is same as number of diffs' do - let(:offset) { compare.diffs.size } - - it 'no diffs are streamed' do - send_request - - expect(response.body).to be_empty - end - end - end - - context 'when an exception occurs' do - before do - allow(::RapidDiffs::DiffFileComponent) - .to receive(:new).and_raise(StandardError.new('something went wrong')) - end - - it 'prints out error message' do - send_request - - expect(response.body).to include('something went wrong') - end - end - - context 'when the rapid_diffs feature flag is disabled' do - before do - stub_feature_flags(rapid_diffs: false) - end - - it 'returns a 404 status' do - send_request - - expect(response).to have_gitlab_http_status(:not_found) - end - end + include_examples 'diffs stream tests' end end diff --git a/spec/support/shared_examples/diffs_stream_shared_examples.rb b/spec/support/shared_examples/diffs_stream_shared_examples.rb new file mode 100644 index 00000000000000..76f8aabd14c283 --- /dev/null +++ b/spec/support/shared_examples/diffs_stream_shared_examples.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'diffs stream tests' do + it 'streams the response' do + send_request + + expect(response).to have_gitlab_http_status(:success) + end + + context 'when offset is given' do + context 'when offset is 1' do + let(:offset) { 1 } + + it 'streams diffs except the offset' do + send_request + + diff_files_array = diff_files.to_a + expect(response.body).not_to include(diff_files_array.first.new_path) + expect(response.body).to include(diff_files_array.last.new_path) + end + end + + context 'when offset is the same as the number of diffs' do + let(:offset) { diff_files.size } + + it 'no diffs are streamed' do + send_request + + expect(response.body).to be_empty + end + end + end + + context 'when an exception occurs' do + before do + allow(::RapidDiffs::DiffFileComponent) + .to receive(:new).and_raise(StandardError.new('something went wrong')) + end + + it 'prints out error message' do + send_request + + expect(response.body).to include('something went wrong') + end + end + + context 'when the rapid_diffs feature flag is disabled' do + before do + stub_feature_flags(rapid_diffs: false) + end + + it 'returns a 404 status' do + send_request + + expect(response).to have_gitlab_http_status(:not_found) + end + end +end -- GitLab From a373e127366bac34bb2b87afede5eac9d506fd75 Mon Sep 17 00:00:00 2001 From: Emma Park Date: Fri, 15 Nov 2024 00:55:21 +0100 Subject: [PATCH 8/8] Test offset_index param --- spec/lib/gitlab/git/diff_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/lib/gitlab/git/diff_spec.rb b/spec/lib/gitlab/git/diff_spec.rb index 13c44afef2fd29..c04ad13953dad3 100644 --- a/spec/lib/gitlab/git/diff_spec.rb +++ b/spec/lib/gitlab/git/diff_spec.rb @@ -354,7 +354,7 @@ end describe '.filter_diff_options' do - let(:options) { { max_files: 100, invalid_opt: true } } + let(:options) { { max_files: 100, invalid_opt: true, offset_index: 10 } } context "without default options" do let(:filtered_options) { described_class.filter_diff_options(options) } -- GitLab