Commit 70d40494 authored by Shinya Maeda's avatar Shinya Maeda

Merge branch 'filter-pipeline-merge-requests-by-sha' into 'master'

Prevent false positives in Ci::Pipeline#all_merge_requests

See merge request gitlab-org/gitlab!28800
parents b67aa714 1fe80693
...@@ -736,6 +736,7 @@ module Ci ...@@ -736,6 +736,7 @@ module Ci
MergeRequest.where(id: merge_request_id) MergeRequest.where(id: merge_request_id)
else else
MergeRequest.where(source_project_id: project_id, source_branch: ref) MergeRequest.where(source_project_id: project_id, source_branch: ref)
.by_commit_sha(sha)
end end
end end
......
---
title: Prevent false positives in Ci::Pipeline#all_merge_requests
merge_request: 28800
author:
type: fixed
...@@ -7,7 +7,7 @@ describe PipelineSerializer do ...@@ -7,7 +7,7 @@ describe PipelineSerializer do
let_it_be(:user) { create(:user) } let_it_be(:user) { create(:user) }
let(:serializer) do let(:serializer) do
described_class.new(current_user: user) described_class.new(current_user: user, project: project)
end end
subject { serializer.represent(pipeline, details: true) } subject { serializer.represent(pipeline, details: true) }
......
...@@ -5,7 +5,7 @@ FactoryBot.define do ...@@ -5,7 +5,7 @@ FactoryBot.define do
factory :ci_empty_pipeline, class: 'Ci::Pipeline' do factory :ci_empty_pipeline, class: 'Ci::Pipeline' do
source { :push } source { :push }
ref { 'master' } ref { 'master' }
sha { '97de212e80737a608d939f648d959671fb0a0142' } sha { 'b83d6e391c22777fca1ed3012fce84f633d7fed0' }
status { 'pending' } status { 'pending' }
add_attribute(:protected) { false } add_attribute(:protected) { false }
......
...@@ -244,13 +244,15 @@ describe 'Dashboard Projects' do ...@@ -244,13 +244,15 @@ describe 'Dashboard Projects' do
ActiveRecord::QueryRecorder.new { visit dashboard_projects_path }.count ActiveRecord::QueryRecorder.new { visit dashboard_projects_path }.count
# There are three known N+1 queries: # There are seven known N+1 queries: https://gitlab.com/gitlab-org/gitlab/-/issues/214037
# 1. Project#open_issues_count # 1. Project#open_issues_count
# 2. Project#open_merge_requests_count # 2. Project#open_merge_requests_count
# 3. Project#forks_count # 3. Project#forks_count
# # 4. ProjectsHelper#load_pipeline_status
# In addition, ProjectsHelper#load_pipeline_status also adds an # 5. RendersMemberAccess#preload_max_member_access_for_collection
# additional query. # 6. User#max_member_access_for_project_ids
expect { visit dashboard_projects_path }.not_to exceed_query_limit(control_count + 4) # 7. CommitWithPipeline#last_pipeline
expect { visit dashboard_projects_path }.not_to exceed_query_limit(control_count + 7)
end end
end end
...@@ -133,15 +133,8 @@ describe 'Pipeline', :js do ...@@ -133,15 +133,8 @@ describe 'Pipeline', :js do
context 'when there are two related merge requests' do context 'when there are two related merge requests' do
before do before do
create(:merge_request, create(:merge_request, source_project: project, source_branch: pipeline.ref)
source_project: project, create(:merge_request, source_project: project, source_branch: pipeline.ref, target_branch: 'fix')
source_branch: pipeline.ref,
target_branch: 'feature-1')
create(:merge_request,
source_project: project,
source_branch: pipeline.ref,
target_branch: 'feature-2')
end end
it 'links to the most recent related merge request' do it 'links to the most recent related merge request' do
......
...@@ -14,7 +14,7 @@ describe Resolvers::MergeRequestPipelinesResolver do ...@@ -14,7 +14,7 @@ describe Resolvers::MergeRequestPipelinesResolver do
sha: merge_request.diff_head_sha sha: merge_request.diff_head_sha
) )
end end
let_it_be(:other_project_pipeline) { create(:ci_pipeline, project: merge_request.source_project) } let_it_be(:other_project_pipeline) { create(:ci_pipeline, project: merge_request.source_project, ref: 'other-ref') }
let_it_be(:other_pipeline) { create(:ci_pipeline) } let_it_be(:other_pipeline) { create(:ci_pipeline) }
let(:current_user) { create(:user) } let(:current_user) { create(:user) }
......
...@@ -2367,18 +2367,31 @@ describe Ci::Pipeline, :mailer do ...@@ -2367,18 +2367,31 @@ describe Ci::Pipeline, :mailer do
end end
end end
describe "#all_merge_requests" do describe '#all_merge_requests' do
let(:project) { create(:project) } let(:project) { create(:project) }
shared_examples 'a method that returns all merge requests for a given pipeline' do shared_examples 'a method that returns all merge requests for a given pipeline' do
let(:pipeline) { create(:ci_empty_pipeline, status: 'created', project: pipeline_project, ref: 'master') } let(:pipeline) { create(:ci_empty_pipeline, status: 'created', project: pipeline_project, ref: 'master') }
it "returns all merge requests having the same source branch" do it 'returns all merge requests having the same source branch and the pipeline sha' do
merge_request = create(:merge_request, source_project: pipeline_project, target_project: project, source_branch: pipeline.ref) merge_request = create(:merge_request, source_project: pipeline_project, target_project: project, source_branch: pipeline.ref)
create(:merge_request_diff, merge_request: merge_request).tap do |diff|
create(:merge_request_diff_commit, merge_request_diff: diff, sha: pipeline.sha)
end
expect(pipeline.all_merge_requests).to eq([merge_request]) expect(pipeline.all_merge_requests).to eq([merge_request])
end end
it "doesn't return merge requests having the same source branch without the pipeline sha" do
merge_request = create(:merge_request, source_project: pipeline_project, target_project: project, source_branch: pipeline.ref)
create(:merge_request_diff, merge_request: merge_request).tap do |diff|
create(:merge_request_diff_commit, merge_request_diff: diff, sha: 'unrelated')
end
expect(pipeline.all_merge_requests).to be_empty
end
it "doesn't return merge requests having a different source branch" do it "doesn't return merge requests having a different source branch" do
create(:merge_request, source_project: pipeline_project, target_project: project, source_branch: 'feature', target_branch: 'master') create(:merge_request, source_project: pipeline_project, target_project: project, source_branch: 'feature', target_branch: 'master')
......
...@@ -236,7 +236,7 @@ describe Ci::PipelinePresenter do ...@@ -236,7 +236,7 @@ describe Ci::PipelinePresenter do
context 'for a branch pipeline with two open MRs' do context 'for a branch pipeline with two open MRs' do
let!(:one) { create(:merge_request, source_project: project, source_branch: pipeline.ref) } let!(:one) { create(:merge_request, source_project: project, source_branch: pipeline.ref) }
let!(:two) { create(:merge_request, source_project: project, source_branch: pipeline.ref, target_branch: 'wip') } let!(:two) { create(:merge_request, source_project: project, source_branch: pipeline.ref, target_branch: 'fix') }
it { is_expected.to contain_exactly(one, two) } it { is_expected.to contain_exactly(one, two) }
end end
......
...@@ -15,7 +15,7 @@ describe BuildDetailsEntity do ...@@ -15,7 +15,7 @@ describe BuildDetailsEntity do
let(:project) { create(:project, :repository) } let(:project) { create(:project, :repository) }
let(:pipeline) { create(:ci_pipeline, project: project) } let(:pipeline) { create(:ci_pipeline, project: project) }
let(:build) { create(:ci_build, :failed, pipeline: pipeline) } let(:build) { create(:ci_build, :failed, pipeline: pipeline) }
let(:request) { double('request') } let(:request) { double('request', project: project) }
let(:entity) do let(:entity) do
described_class.new(build, request: request, described_class.new(build, request: request,
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment