From 61ebb727252d6398f6c1233d9e6ed50058c650b9 Mon Sep 17 00:00:00 2001 From: egrieff Date: Tue, 30 Sep 2025 17:06:48 +0200 Subject: [PATCH] Update MergeRequestDiff#includes_any_commits? method In `MergeRequestDiff#includes_any_commits?`, we search commits SHAs from the `merge_request_diff_commits` table. Since the column `merge_request_diff_commits.sha` will be moved to the `merge_request_commits_metadata` table, we need to update the query to look in this table as well. We revert to the original behaviour when the feature flag `merge_request_diff_commits_dedup` is disabled or the table `merge_request_commits_metadata` has no records for diff's commits. --- app/models/merge_request_diff.rb | 15 ++++ spec/models/merge_request_diff_spec.rb | 95 +++++++++++++++++++++----- 2 files changed, 94 insertions(+), 16 deletions(-) diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index da822527020ef1..fafaec15ea3aee 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -444,6 +444,8 @@ def includes_any_commits?(shas) # when the number of shas is huge (1000+) we don't want # to pass them all as an SQL param, let's pass them in batches shas.each_slice(BATCH_SIZE).any? do |batched_shas| + break true if diff_commits_dedup_enabled? && metadata_sha_exists?(batched_shas) + merge_request_diff_commits.where(sha: batched_shas).exists? end end @@ -1043,6 +1045,19 @@ def external_diff_cache_dir ) end + def metadata_sha_exists?(shas) + MergeRequest::CommitsMetadata + .where(project: project, sha: shas) + .where_exists( + MergeRequestDiffCommit + .where(merge_request_diff_id: id) + .where( + MergeRequestDiffCommit.arel_table[:merge_request_commits_metadata_id] + .eq(MergeRequest::CommitsMetadata.arel_table[:id]) + ) + ).exists? + end + def diff_commits_dedup_enabled? Feature.enabled?(:merge_request_diff_commits_dedup, project) end diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb index c7e7bc720aadeb..24b546eb1b02ce 100644 --- a/spec/models/merge_request_diff_spec.rb +++ b/spec/models/merge_request_diff_spec.rb @@ -1609,37 +1609,100 @@ end describe '#includes_any_commits?' do + let_it_be(:project) { create(:project, :repository) } + let_it_be_with_refind(:merge_request_diff) do + create(:merge_request, source_project: project, target_project: project).merge_request_diff + end + + let_it_be(:commits_metadata) { create(:merge_request_commits_metadata, project: project, sha: 'abc123') } + + let_it_be(:diff_commit_with_metadata) do + create(:merge_request_diff_commit, + merge_request_diff: merge_request_diff, + merge_request_commits_metadata_id: commits_metadata.id, + relative_order: merge_request_diff.merge_request_diff_commits.count + 1, + sha: nil + ) + end + + let_it_be(:diff_commit_without_metadata) do + create(:merge_request_diff_commit, + merge_request_diff: merge_request_diff, + relative_order: merge_request_diff.merge_request_diff_commits.count + 1, + sha: 'def456' + ) + end + let(:non_existent_shas) do Array.new(30) { Digest::SHA1.hexdigest(SecureRandom.hex) } end - subject { diff_with_commits } + shared_examples 'merge request diff with commit shas' do + let(:args_with_existing_commits) { non_existent_shas + existing_shas } - context 'processes the passed shas in batches' do - context 'number of existing commits is greater than batch size' do - it 'performs a separate request for each batch' do - stub_const('MergeRequestDiff::BATCH_SIZE', 5) + it 'performs a separate request for each batch' do + stub_const('MergeRequestDiff::BATCH_SIZE', 5) - commit_shas = subject.commit_shas + recorder = ActiveRecord::QueryRecorder.new do + merge_request_diff.includes_any_commits?(args_with_existing_commits) + end - query_count = ActiveRecord::QueryRecorder.new do - subject.includes_any_commits?(non_existent_shas + commit_shas) - end.count + metadata_queries = recorder.log.select do |q| + q.include?("SELECT 1 AS one FROM \"merge_request_commits_metadata\"") + end - expect(query_count).to eq(7) + commit_queries = recorder.log.select do |q| + q.include?("SELECT 1 AS one FROM \"merge_request_diff_commits\"") end + + expect(metadata_queries.count).to eq(expected_metadata_queries) + expect(commit_queries.count).to eq(expected_commit_queries) + end + + it 'returns false if passed commits do not exist' do + expect(merge_request_diff.includes_any_commits?([])).to eq(false) + expect(merge_request_diff.includes_any_commits?([Gitlab::Git::SHA1_BLANK_SHA])).to eq(false) end + + it 'returns true if passed commits exists' do + expect(merge_request_diff.includes_any_commits?(args_with_existing_commits)).to eq(true) + end + end + + context 'when SHA is present in `merge_request_diff_commits` table' do + let(:expected_metadata_queries) { 7 } + let(:expected_commit_queries) { 7 } + let(:existing_shas) { ['def456'] } + + it_behaves_like 'merge request diff with commit shas' end - it 'returns false if passed commits do not exist' do - expect(subject.includes_any_commits?([])).to eq(false) - expect(subject.includes_any_commits?([Gitlab::Git::SHA1_BLANK_SHA])).to eq(false) + context 'when SHA is present in `merge_request_commits_metadata` table' do + let(:expected_metadata_queries) { 7 } + let(:expected_commit_queries) { 6 } + let(:existing_shas) { ['abc123'] } + + it_behaves_like 'merge request diff with commit shas' + end + + context 'when `sha` data is present across both tables' do + let(:expected_metadata_queries) { 7 } + let(:expected_commit_queries) { 6 } + let(:existing_shas) { %w[abc123 def456] } + + it_behaves_like 'merge request diff with commit shas' end - it 'returns true if passed commits exists' do - args_with_existing_commits = non_existent_shas << subject.head_commit_sha + context 'when merge_request_diff_commits_dedup feature flag is disabled' do + let(:existing_shas) { ['def456'] } + let(:expected_commit_queries) { 7 } + let(:expected_metadata_queries) { 0 } + + before do + stub_feature_flags(merge_request_diff_commits_dedup: false) + end - expect(subject.includes_any_commits?(args_with_existing_commits)).to eq(true) + it_behaves_like 'merge request diff with commit shas' end end -- GitLab