diff --git a/app/controllers/concerns/diffs_stream_resource.rb b/app/controllers/concerns/diffs_stream_resource.rb deleted file mode 100644 index 73d5a1f12e161431dbac50be368f67428cee0277..0000000000000000000000000000000000000000 --- a/app/controllers/concerns/diffs_stream_resource.rb +++ /dev/null @@ -1,17 +0,0 @@ -# frozen_string_literal: true - -module DiffsStreamResource - extend ActiveSupport::Concern - - def diffs_stream_url(resource, offset = nil, diff_view = nil) - return if offset && offset > resource.diffs_for_streaming.diff_files.count - - diffs_stream_resource_url(resource, offset, diff_view) - end - - private - - def diffs_stream_resource_url(resource, offset, diff_view) - raise NotImplementedError - end -end diff --git a/app/controllers/concerns/rapid_diffs_resource.rb b/app/controllers/concerns/rapid_diffs_resource.rb new file mode 100644 index 0000000000000000000000000000000000000000..f302a9b74a95cf83362390012007eafac13dea4f --- /dev/null +++ b/app/controllers/concerns/rapid_diffs_resource.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +module RapidDiffsResource + extend ActiveSupport::Concern + + def diffs_stream_url(resource, offset = nil, diff_view = nil) + return if offset && offset > resource.diffs_for_streaming.diff_files.count + + diffs_stream_resource_url(resource, offset, diff_view) + end + + def diff_files_metadata + return render_404 unless rapid_diffs_enabled? + return render_404 unless diffs_resource.present? + + render json: { + diff_files: DiffFileMetadataEntity.represent(diffs_resource.raw_diff_files(sorted: true)) + } + end + + private + + def rapid_diffs_enabled? + ::Feature.enabled?(:rapid_diffs, current_user, type: :wip) + end + + def diffs_resource + raise NotImplementedError + end + + def diffs_stream_resource_url(resource, offset, diff_view) + raise NotImplementedError + end +end diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb index 1e0dd40b687d857c9252130c118041017d76884a..2adaff0cfd00b5a5527abbe99abc25184c30d7a0 100644 --- a/app/controllers/projects/commit_controller.rb +++ b/app/controllers/projects/commit_controller.rb @@ -9,7 +9,7 @@ class Projects::CommitController < Projects::ApplicationController include DiffForPath include DiffHelper include SourcegraphDecorator - include DiffsStreamResource + include RapidDiffsResource # Authorize before_action :require_non_empty_project @@ -298,6 +298,10 @@ def rate_limit_for_expanded_diff_files check_rate_limit!(:expanded_diff_files, scope: current_user || request.ip) end + + def diffs_resource + commit&.diffs(commit_diff_options) + end end Projects::CommitController.prepend_mod_with('Projects::CommitController') diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb index f42791499502e0fe4c6cf2d5cbf8f2c5fd0bf5cb..3157d7ab6f8f9cc4d77a0858c2778dff91bd5fdd 100644 --- a/app/controllers/projects/compare_controller.rb +++ b/app/controllers/projects/compare_controller.rb @@ -7,6 +7,7 @@ class Projects::CompareController < Projects::ApplicationController include DiffHelper include RendersCommits include CompareHelper + include RapidDiffsResource # Authorize before_action :require_non_empty_project @@ -195,4 +196,8 @@ def merge_request def compare_params @compare_params ||= params.permit(:from, :to, :from_project_id, :straight, :to_project_id) end + + def diffs_resource + compare&.diffs(diff_options) + end end diff --git a/app/controllers/projects/merge_requests/creations_controller.rb b/app/controllers/projects/merge_requests/creations_controller.rb index 401c873971911cdcef7b8006ffa5610f28cb9db7..e88cbfa00c0222ab6fac73bfb3ce5d76866dd811 100644 --- a/app/controllers/projects/merge_requests/creations_controller.rb +++ b/app/controllers/projects/merge_requests/creations_controller.rb @@ -5,6 +5,7 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap include DiffHelper include RendersCommits include ProductAnalyticsTracking + include RapidDiffsResource skip_before_action :merge_request before_action :authorize_create_merge_request_from! @@ -105,6 +106,10 @@ def target_projects render json: ProjectSerializer.new.represent(get_target_projects) end + def diffs_resource + @merge_request&.compare&.diffs + end + private def get_target_projects diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 0e450f56bb7b01e5894ce0f371ad2264a2877a4a..f529b327d6f165f925a9e2a0f976c182612b5bcb 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -13,7 +13,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo include Gitlab::Cache::Helpers include MergeRequestsHelper include ParseCommitDate - include DiffsStreamResource + include RapidDiffsResource prepend_before_action(only: [:index]) { authenticate_sessionless_user!(:rss) } skip_before_action :merge_request, only: [:index, :bulk_update, :export_csv] @@ -693,6 +693,10 @@ def display_max_limit_warning _("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 + + def diffs_resource + @merge_request.latest_diffs + end end Projects::MergeRequestsController.prepend_mod_with('Projects::MergeRequestsController') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index a5ff789d77fa20c88bcd57fdba3fa32f8d164698..4bcfdc737ad6cc86fc4dcc8dcb8f0dea0e35c648 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -901,6 +901,12 @@ def diffs_for_streaming(diff_options = {}, &) end end + def latest_diffs + diff = diffable_merge_ref? ? merge_head_diff : merge_request_diff + + diff.diffs(diff_options) + end + def diffs(diff_options = {}) if compare # When saving MR diffs, `expanded` is implicitly added (because we need diff --git a/config/routes/merge_requests.rb b/config/routes/merge_requests.rb index 252a13c7ad1da69d18d68fafbd40ca0a14a6ae69..ee4e2e5d529e274b5ad34a3353c2b2cbbed1fa13 100644 --- a/config/routes/merge_requests.rb +++ b/config/routes/merge_requests.rb @@ -45,6 +45,7 @@ get :diff_for_path, controller: 'merge_requests/diffs' get 'diff_by_file_hash/:file_hash', to: 'merge_requests/diffs#diff_by_file_hash', as: :diff_by_file_hash get :diffs_stream, to: 'merge_requests/diffs_stream#diffs' + get :diff_files_metadata # NOTE: Fallback to `merge_requests/diffs#diff_for_path` to handle `collapsed_diff_url` from the collapsed partial scope controller: 'merge_requests/diffs_stream' do @@ -95,5 +96,6 @@ get :branch_from get :branch_to get :diffs_stream, to: 'merge_requests/creations_diffs_stream#diffs' + get :diff_files_metadata end end diff --git a/config/routes/repository.rb b/config/routes/repository.rb index cf1a823039843ac91871c90c8619b5fe06bd17a1..37e28eb5df0717647c874b81affb467ed5840628 100644 --- a/config/routes/repository.rb +++ b/config/routes/repository.rb @@ -19,6 +19,7 @@ get :diff_for_path get :signatures get :diffs_stream, to: 'compare_diffs_stream#diffs' + get :diff_files_metadata end end @@ -112,6 +113,7 @@ get :diff_for_path get :diff_files get :merge_requests + get :diff_files_metadata end end diff --git a/spec/controllers/concerns/diffs_stream_resource_spec.rb b/spec/controllers/concerns/rapid_diffs_resource_spec.rb similarity index 75% rename from spec/controllers/concerns/diffs_stream_resource_spec.rb rename to spec/controllers/concerns/rapid_diffs_resource_spec.rb index db86c7c5449e07a49b8a63bcb238e3e46e57a688..34505b3b9d5b927cd7c7c9277ecd82c619974aa5 100644 --- a/spec/controllers/concerns/diffs_stream_resource_spec.rb +++ b/spec/controllers/concerns/rapid_diffs_resource_spec.rb @@ -2,10 +2,10 @@ require 'spec_helper' -RSpec.describe DiffsStreamResource, type: :controller, feature_category: :source_code_management do +RSpec.describe RapidDiffsResource, type: :controller, feature_category: :source_code_management do subject(:controller) do Class.new(ApplicationController) do - include DiffsStreamResource + include RapidDiffsResource def call_diffs_stream_resource_url(resource, offset, diff_view) diffs_stream_resource_url(resource, offset, diff_view) @@ -14,6 +14,10 @@ def call_diffs_stream_resource_url(resource, offset, diff_view) def call_diffs_stream_url(resource, offset, diff_view) diffs_stream_url(resource, offset, diff_view) end + + def call_diffs_resource + diffs_resource + end end end @@ -40,4 +44,12 @@ def call_diffs_stream_url(resource, offset, diff_view) end end end + + describe '#diffs_resource' do + it 'raises NotImplementedError' do + expect do + controller.new.call_diffs_resource + end.to raise_error(NotImplementedError) + end + end end diff --git a/spec/controllers/projects/compare_controller_spec.rb b/spec/controllers/projects/compare_controller_spec.rb index 6e1133dc8c54074d26f53eb51ff6db97e9af135a..ead2bc4a6d97320657eca207551554d3d55bb0cc 100644 --- a/spec/controllers/projects/compare_controller_spec.rb +++ b/spec/controllers/projects/compare_controller_spec.rb @@ -771,5 +771,32 @@ expect(response).to have_gitlab_http_status(:not_found) end end + + describe 'Get #diff_files_metadata' do + let(:params) do + { + namespace_id: project.namespace, + project_id: project, + from: from, + to: to + } + end + + let(:send_request) { get :diff_files_metadata, params: params } + + context 'with valid params' do + let(:from) { '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9' } + let(:to) { '5937ac0a7beb003549fc5fd26fc247adbce4a52e' } + + include_examples 'diff files metadata' + end + + context 'with invalid params' do + let(:from) { '0123456789' } + let(:to) { '987654321' } + + include_examples 'missing diff files metadata' + end + end end end diff --git a/spec/requests/projects/commit_controller_spec.rb b/spec/requests/projects/commit_controller_spec.rb index a42fa7a37813555c433856b005636dfbcae8baaf..58442c615b0b7e38bd254f8436b0aac5e4d63309 100644 --- a/spec/requests/projects/commit_controller_spec.rb +++ b/spec/requests/projects/commit_controller_spec.rb @@ -94,6 +94,34 @@ end end + describe 'GET #diff_files_metadata' do + let(:params) do + { + namespace_id: project.namespace, + project_id: project, + id: sha + } + end + + let(:send_request) { get diff_files_metadata_namespace_project_commit_path(params) } + + before do + sign_in(user) + end + + context 'with valid params' do + let(:sha) { '7d3b0f7cff5f37573aea97cebfd5692ea1689924' } + + include_examples 'diff files metadata' + end + + context 'with invalid params' do + let(:sha) { '0123456789' } + + include_examples 'missing diff files metadata' + end + end + describe 'GET #diff_files' do let(:master_pickable_sha) { '7d3b0f7cff5f37573aea97cebfd5692ea1689924' } let(:format) { :html } diff --git a/spec/requests/projects/merge_requests/creations_spec.rb b/spec/requests/projects/merge_requests/creations_spec.rb index 93a5e6cf5bc7908eb7ed8c7805a0ff8082eaa07d..5629f451f5bdc51c59eac6cfb1ee16ba3570961e 100644 --- a/spec/requests/projects/merge_requests/creations_spec.rb +++ b/spec/requests/projects/merge_requests/creations_spec.rb @@ -76,6 +76,36 @@ def get_new(params = get_params) end end end + + describe 'GET new/diff_files_metadata' do + let(:send_request) { get namespace_project_new_merge_request_diff_files_metadata_path(params) } + + context 'with valid params' do + let(:params) do + { + namespace_id: project.namespace.to_param, + project_id: project, + merge_request: { + source_branch: 'fix', + target_branch: 'master' + } + } + end + + include_examples 'diff files metadata' + end + + context 'with invalid params' do + let(:params) do + { + namespace_id: project.namespace.to_param, + project_id: project + } + end + + include_examples 'missing diff files metadata' + end + end end describe 'POST /:namespace/:project/merge_requests' do diff --git a/spec/requests/projects/merge_requests_controller_spec.rb b/spec/requests/projects/merge_requests_controller_spec.rb index 7555669b23b1212ba2dc808035736951cab393ab..3ee1404aa71c76fac0c0cb2b57530c43333dbc1c 100644 --- a/spec/requests/projects/merge_requests_controller_spec.rb +++ b/spec/requests/projects/merge_requests_controller_spec.rb @@ -286,6 +286,28 @@ def create_pipeline end end + describe 'GET #diff_files_metadata' do + before do + project.add_developer(user) + login_as(user) + end + + let(:send_request) { get diff_files_metadata_project_merge_request_path(project, merge_request) } + + include_examples 'diff files metadata' + + context 'when merge_request_diff does not exist' do + let(:merge_request) { create(:merge_request, :skip_diff_creation, author: user) } + let(:project) { merge_request.project } + + it 'returns an empty array' do + send_request + + expect(json_response['diff_files']).to be_empty + end + end + end + describe 'PUT #update' do before do project.add_developer(user) diff --git a/spec/support/shared_examples/diff_files_metadata_shared_examples.rb b/spec/support/shared_examples/diff_files_metadata_shared_examples.rb new file mode 100644 index 0000000000000000000000000000000000000000..ef02a0313f274e47b93064ca43e24a7ec1b6fb95 --- /dev/null +++ b/spec/support/shared_examples/diff_files_metadata_shared_examples.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'diff files metadata' do + it 'returns a json response' do + send_request + + expect(response).to have_gitlab_http_status(:success) + expect(json_response['diff_files']).to be_an Array + 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 + +RSpec.shared_examples 'missing diff files metadata' do + it 'returns a 404 status' do + send_request + + expect(response).to have_gitlab_http_status(:not_found) + end +end