From 258c2d51a0cc50bd2c5fc46757f3c48101c6b8e1 Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Mon, 16 Dec 2024 16:58:55 +0800 Subject: [PATCH 1/4] Implement diff_files_metadata endpoints This adds `diff_files_metadata` endpoint for each resource that we support in rapid diffs (e.g. `MergeRequest`, `Commit` and `Compare`). This is behind `rapid_diffs` FF. --- app/controllers/concerns/diffs_stream_resource.rb | 14 ++++++++++++++ app/controllers/projects/commit_controller.rb | 4 ++++ app/controllers/projects/compare_controller.rb | 5 +++++ .../projects/merge_requests_controller.rb | 4 ++++ app/models/merge_request.rb | 6 ++++++ config/routes/merge_requests.rb | 1 + config/routes/repository.rb | 2 ++ 7 files changed, 36 insertions(+) diff --git a/app/controllers/concerns/diffs_stream_resource.rb b/app/controllers/concerns/diffs_stream_resource.rb index 73d5a1f12e1614..ca9660de78db2a 100644 --- a/app/controllers/concerns/diffs_stream_resource.rb +++ b/app/controllers/concerns/diffs_stream_resource.rb @@ -9,8 +9,22 @@ def diffs_stream_url(resource, offset = nil, diff_view = nil) diffs_stream_resource_url(resource, offset, diff_view) end + def diff_files_metadata + return render_404 unless rapid_diffs_enabled? + + render json: 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 diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb index 1e0dd40b687d85..ec650282b44ad0 100644 --- a/app/controllers/projects/commit_controller.rb +++ b/app/controllers/projects/commit_controller.rb @@ -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 f42791499502e0..613348fdf9a729 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 DiffsStreamResource # 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_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 244267bfadd68d..47db1d5be79156 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -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 cfea6a5559278a..6a300539d972ac 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 252a13c7ad1da6..ca6032f6b4c1d6 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 diff --git a/config/routes/repository.rb b/config/routes/repository.rb index cf1a823039843a..37e28eb5df0717 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 -- GitLab From d64192b1d6c5a830b7ce874f7927bb23cf813e46 Mon Sep 17 00:00:00 2001 From: David Kim Date: Fri, 7 Mar 2025 19:18:06 +1100 Subject: [PATCH 2/4] Add diff_files_metadata to MR creation and add specs --- .../concerns/diffs_stream_resource.rb | 1 + app/controllers/projects/commit_controller.rb | 2 +- .../projects/compare_controller.rb | 2 +- .../merge_requests/creations_controller.rb | 5 ++++ config/routes/merge_requests.rb | 1 + .../projects/compare_controller_spec.rb | 27 +++++++++++++++++ .../projects/commit_controller_spec.rb | 28 +++++++++++++++++ .../projects/merge_requests/creations_spec.rb | 30 +++++++++++++++++++ .../merge_requests_controller_spec.rb | 6 ++++ .../diff_files_metadata_shared_examples.rb | 30 +++++++++++++++++++ 10 files changed, 130 insertions(+), 2 deletions(-) create mode 100644 spec/support/shared_examples/diff_files_metadata_shared_examples.rb diff --git a/app/controllers/concerns/diffs_stream_resource.rb b/app/controllers/concerns/diffs_stream_resource.rb index ca9660de78db2a..d81a30b3d216e9 100644 --- a/app/controllers/concerns/diffs_stream_resource.rb +++ b/app/controllers/concerns/diffs_stream_resource.rb @@ -11,6 +11,7 @@ def diffs_stream_url(resource, offset = nil, diff_view = nil) def diff_files_metadata return render_404 unless rapid_diffs_enabled? + return render_404 unless diffs_resource.present? render json: DiffFileMetadataEntity.represent(diffs_resource.raw_diff_files(sorted: true)) end diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb index ec650282b44ad0..4cdfa3d0ee5ce3 100644 --- a/app/controllers/projects/commit_controller.rb +++ b/app/controllers/projects/commit_controller.rb @@ -300,7 +300,7 @@ def rate_limit_for_expanded_diff_files end def diffs_resource - commit.diffs(commit_diff_options) + commit&.diffs(commit_diff_options) end end diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb index 613348fdf9a729..464f251fbbff5d 100644 --- a/app/controllers/projects/compare_controller.rb +++ b/app/controllers/projects/compare_controller.rb @@ -198,6 +198,6 @@ def compare_params end def diffs_resource - compare.diffs(diff_options) + 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 401c873971911c..c8cd9530402ae4 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 DiffsStreamResource 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/config/routes/merge_requests.rb b/config/routes/merge_requests.rb index ca6032f6b4c1d6..ee4e2e5d529e27 100644 --- a/config/routes/merge_requests.rb +++ b/config/routes/merge_requests.rb @@ -96,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/spec/controllers/projects/compare_controller_spec.rb b/spec/controllers/projects/compare_controller_spec.rb index 6e1133dc8c5407..0e84f3e5e31e9e 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 'invalid 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 a42fa7a3781355..3410b1fda19954 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 'invalid 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 93a5e6cf5bc790..d984bf98333509 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 'invalid 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 7555669b23b121..502df866c13baa 100644 --- a/spec/requests/projects/merge_requests_controller_spec.rb +++ b/spec/requests/projects/merge_requests_controller_spec.rb @@ -274,6 +274,12 @@ def get_discussions(**params) end end + describe 'GET #diff_files_metadata' do + let(:send_request) { get diff_files_metadata_project_merge_request_path(project, merge_request) } + + include_examples 'diff files metadata' + end + private def create_pipeline 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 00000000000000..f0e562434fe02d --- /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).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 'invalid diff files metadata' do + it 'returns a 404 status' do + send_request + + expect(response).to have_gitlab_http_status(:not_found) + end +end -- GitLab From 14f9bb88bb323f3d9cfccb15a2a49833f0a74437 Mon Sep 17 00:00:00 2001 From: David Kim Date: Tue, 11 Mar 2025 18:34:18 +1100 Subject: [PATCH 3/4] Address review comments --- ...am_resource.rb => rapid_diffs_resource.rb} | 6 ++-- app/controllers/projects/commit_controller.rb | 2 +- .../projects/compare_controller.rb | 2 +- .../merge_requests/creations_controller.rb | 2 +- .../projects/merge_requests_controller.rb | 2 +- ...e_spec.rb => rapid_diffs_resource_spec.rb} | 4 +-- .../projects/compare_controller_spec.rb | 2 +- .../projects/commit_controller_spec.rb | 2 +- .../projects/merge_requests/creations_spec.rb | 2 +- .../merge_requests_controller_spec.rb | 28 +++++++++++++++---- .../diff_files_metadata_shared_examples.rb | 4 +-- 11 files changed, 37 insertions(+), 19 deletions(-) rename app/controllers/concerns/{diffs_stream_resource.rb => rapid_diffs_resource.rb} (82%) rename spec/controllers/concerns/{diffs_stream_resource_spec.rb => rapid_diffs_resource_spec.rb} (89%) diff --git a/app/controllers/concerns/diffs_stream_resource.rb b/app/controllers/concerns/rapid_diffs_resource.rb similarity index 82% rename from app/controllers/concerns/diffs_stream_resource.rb rename to app/controllers/concerns/rapid_diffs_resource.rb index d81a30b3d216e9..f302a9b74a95cf 100644 --- a/app/controllers/concerns/diffs_stream_resource.rb +++ b/app/controllers/concerns/rapid_diffs_resource.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -module DiffsStreamResource +module RapidDiffsResource extend ActiveSupport::Concern def diffs_stream_url(resource, offset = nil, diff_view = nil) @@ -13,7 +13,9 @@ def diff_files_metadata return render_404 unless rapid_diffs_enabled? return render_404 unless diffs_resource.present? - render json: DiffFileMetadataEntity.represent(diffs_resource.raw_diff_files(sorted: true)) + render json: { + diff_files: DiffFileMetadataEntity.represent(diffs_resource.raw_diff_files(sorted: true)) + } end private diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb index 4cdfa3d0ee5ce3..2adaff0cfd00b5 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 diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb index 464f251fbbff5d..3157d7ab6f8f9c 100644 --- a/app/controllers/projects/compare_controller.rb +++ b/app/controllers/projects/compare_controller.rb @@ -7,7 +7,7 @@ class Projects::CompareController < Projects::ApplicationController include DiffHelper include RendersCommits include CompareHelper - include DiffsStreamResource + include RapidDiffsResource # Authorize before_action :require_non_empty_project diff --git a/app/controllers/projects/merge_requests/creations_controller.rb b/app/controllers/projects/merge_requests/creations_controller.rb index c8cd9530402ae4..e88cbfa00c0222 100644 --- a/app/controllers/projects/merge_requests/creations_controller.rb +++ b/app/controllers/projects/merge_requests/creations_controller.rb @@ -5,7 +5,7 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap include DiffHelper include RendersCommits include ProductAnalyticsTracking - include DiffsStreamResource + include RapidDiffsResource skip_before_action :merge_request before_action :authorize_create_merge_request_from! diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 47db1d5be79156..98089daca18656 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] diff --git a/spec/controllers/concerns/diffs_stream_resource_spec.rb b/spec/controllers/concerns/rapid_diffs_resource_spec.rb similarity index 89% rename from spec/controllers/concerns/diffs_stream_resource_spec.rb rename to spec/controllers/concerns/rapid_diffs_resource_spec.rb index db86c7c5449e07..727d86080dcb74 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) diff --git a/spec/controllers/projects/compare_controller_spec.rb b/spec/controllers/projects/compare_controller_spec.rb index 0e84f3e5e31e9e..ead2bc4a6d9732 100644 --- a/spec/controllers/projects/compare_controller_spec.rb +++ b/spec/controllers/projects/compare_controller_spec.rb @@ -795,7 +795,7 @@ let(:from) { '0123456789' } let(:to) { '987654321' } - include_examples 'invalid diff files metadata' + include_examples 'missing diff files metadata' end end end diff --git a/spec/requests/projects/commit_controller_spec.rb b/spec/requests/projects/commit_controller_spec.rb index 3410b1fda19954..58442c615b0b7e 100644 --- a/spec/requests/projects/commit_controller_spec.rb +++ b/spec/requests/projects/commit_controller_spec.rb @@ -118,7 +118,7 @@ context 'with invalid params' do let(:sha) { '0123456789' } - include_examples 'invalid diff files metadata' + include_examples 'missing diff files metadata' end end diff --git a/spec/requests/projects/merge_requests/creations_spec.rb b/spec/requests/projects/merge_requests/creations_spec.rb index d984bf98333509..5629f451f5bdc5 100644 --- a/spec/requests/projects/merge_requests/creations_spec.rb +++ b/spec/requests/projects/merge_requests/creations_spec.rb @@ -103,7 +103,7 @@ def get_new(params = get_params) } end - include_examples 'invalid diff files metadata' + include_examples 'missing diff files metadata' end end end diff --git a/spec/requests/projects/merge_requests_controller_spec.rb b/spec/requests/projects/merge_requests_controller_spec.rb index 502df866c13baa..3ee1404aa71c76 100644 --- a/spec/requests/projects/merge_requests_controller_spec.rb +++ b/spec/requests/projects/merge_requests_controller_spec.rb @@ -274,12 +274,6 @@ def get_discussions(**params) end end - describe 'GET #diff_files_metadata' do - let(:send_request) { get diff_files_metadata_project_merge_request_path(project, merge_request) } - - include_examples 'diff files metadata' - end - private def create_pipeline @@ -292,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 index f0e562434fe02d..ef02a0313f274e 100644 --- a/spec/support/shared_examples/diff_files_metadata_shared_examples.rb +++ b/spec/support/shared_examples/diff_files_metadata_shared_examples.rb @@ -5,7 +5,7 @@ send_request expect(response).to have_gitlab_http_status(:success) - expect(json_response).to be_an Array + expect(json_response['diff_files']).to be_an Array end context 'when the rapid_diffs feature flag is disabled' do @@ -21,7 +21,7 @@ end end -RSpec.shared_examples 'invalid diff files metadata' do +RSpec.shared_examples 'missing diff files metadata' do it 'returns a 404 status' do send_request -- GitLab From cfed263377566c870de41be7eeda1df7556c1a2e Mon Sep 17 00:00:00 2001 From: David Kim Date: Wed, 12 Mar 2025 13:08:01 +1100 Subject: [PATCH 4/4] Add a spec to resolve undercoverage issue --- .../concerns/rapid_diffs_resource_spec.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/spec/controllers/concerns/rapid_diffs_resource_spec.rb b/spec/controllers/concerns/rapid_diffs_resource_spec.rb index 727d86080dcb74..34505b3b9d5b92 100644 --- a/spec/controllers/concerns/rapid_diffs_resource_spec.rb +++ b/spec/controllers/concerns/rapid_diffs_resource_spec.rb @@ -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 -- GitLab