From c73da6c1e73f04ece18b5fca5ccd67bf918682f8 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Thu, 16 Aug 2018 11:07:43 +0100 Subject: Move Issue#{referenced,closed_by}_merge_requests to service These methods don't really need to be on the Issue model. Issue#related_branches can also be moved to a service, but we can do that in a separate commit. This commit does not change any behaviour; it just moves code around, renames the service, and refactors the specs. --- spec/models/issue_spec.rb | 99 ++-------------------- ...fetch_referenced_merge_requests_service_spec.rb | 35 -------- .../referenced_merge_requests_service_spec.rb | 72 ++++++++++++++++ spec/support/helpers/cycle_analytics_helpers.rb | 4 +- 4 files changed, 81 insertions(+), 129 deletions(-) delete mode 100644 spec/services/issues/fetch_referenced_merge_requests_service_spec.rb create mode 100644 spec/services/issues/referenced_merge_requests_service_spec.rb (limited to 'spec') diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index 84edfc3ff00..c21d85fb2a4 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -188,98 +188,6 @@ describe Issue do end end - describe '#closed_by_merge_requests' do - let(:project) { create(:project, :repository) } - let(:issue) { create(:issue, project: project)} - let(:closed_issue) { build(:issue, :closed, project: project)} - - let(:mr) do - opts = { - title: 'Awesome merge_request', - description: "Fixes #{issue.to_reference}", - source_branch: 'feature', - target_branch: 'master' - } - MergeRequests::CreateService.new(project, project.owner, opts).execute - end - - let(:closed_mr) do - opts = { - title: 'Awesome merge_request 2', - description: "Fixes #{issue.to_reference}", - source_branch: 'feature', - target_branch: 'master', - state: 'closed' - } - MergeRequests::CreateService.new(project, project.owner, opts).execute - end - - it 'returns the merge request to close this issue' do - expect(issue.closed_by_merge_requests(mr.author)).to eq([mr]) - end - - it "returns an empty array when the merge request is closed already" do - expect(issue.closed_by_merge_requests(closed_mr.author)).to eq([]) - end - - it "returns an empty array when the current issue is closed already" do - expect(closed_issue.closed_by_merge_requests(closed_issue.author)).to eq([]) - end - end - - describe '#referenced_merge_requests' do - let(:project) { create(:project, :public) } - let(:issue) do - create(:issue, description: merge_request.to_reference, project: project) - end - let!(:merge_request) do - create(:merge_request, - source_project: project, - source_branch: 'master', - target_branch: 'feature') - end - - it 'returns the referenced merge requests' do - mr2 = create(:merge_request, - source_project: project, - source_branch: 'feature', - target_branch: 'master') - - create(:note_on_issue, - noteable: issue, - note: mr2.to_reference, - project_id: project.id) - - expect(issue.referenced_merge_requests).to eq([merge_request, mr2]) - end - - it 'returns cross project referenced merge requests' do - other_project = create(:project, :public) - cross_project_merge_request = create(:merge_request, source_project: other_project) - create(:note_on_issue, - noteable: issue, - note: cross_project_merge_request.to_reference(issue.project), - project_id: issue.project.id) - - expect(issue.referenced_merge_requests).to eq([merge_request, cross_project_merge_request]) - end - - it 'excludes cross project references if the user cannot read cross project' do - user = create(:user) - allow(Ability).to receive(:allowed?).and_call_original - expect(Ability).to receive(:allowed?).with(user, :read_cross_project) { false } - - other_project = create(:project, :public) - cross_project_merge_request = create(:merge_request, source_project: other_project) - create(:note_on_issue, - noteable: issue, - note: cross_project_merge_request.to_reference(issue.project), - project_id: issue.project.id) - - expect(issue.referenced_merge_requests(user)).to eq([merge_request]) - end - end - describe '#can_move?' do let(:user) { create(:user) } let(:issue) { create(:issue) } @@ -365,7 +273,12 @@ describe Issue do source_project: subject.project, source_branch: "#{subject.iid}-branch" }) merge_request.create_cross_references!(user) - expect(subject.referenced_merge_requests(user)).not_to be_empty + + referenced_merge_requests = Issues::ReferencedMergeRequestsService + .new(subject.project, user) + .referenced_merge_requests(subject) + + expect(referenced_merge_requests).not_to be_empty expect(subject.related_branches(user)).to eq([subject.to_branch_name]) end diff --git a/spec/services/issues/fetch_referenced_merge_requests_service_spec.rb b/spec/services/issues/fetch_referenced_merge_requests_service_spec.rb deleted file mode 100644 index 4e58179f45f..00000000000 --- a/spec/services/issues/fetch_referenced_merge_requests_service_spec.rb +++ /dev/null @@ -1,35 +0,0 @@ -require 'spec_helper.rb' - -describe Issues::FetchReferencedMergeRequestsService do - let(:project) { create(:project) } - let(:issue) { create(:issue, project: project) } - let(:other_project) { create(:project) } - - let(:mr) { create(:merge_request, source_project: project, target_project: project, id: 2)} - let(:other_mr) { create(:merge_request, source_project: other_project, target_project: other_project, id: 1)} - - let(:user) { create(:user) } - let(:service) { described_class.new(project, user) } - - context 'with mentioned merge requests' do - it 'returns a list of sorted merge requests' do - allow(issue).to receive(:referenced_merge_requests).with(user).and_return([other_mr, mr]) - - mrs, closed_by_mrs = service.execute(issue) - - expect(mrs).to match_array([mr, other_mr]) - expect(closed_by_mrs).to match_array([]) - end - end - - context 'with closed-by merge requests' do - it 'returns a list of sorted merge requests' do - allow(issue).to receive(:closed_by_merge_requests).with(user).and_return([other_mr, mr]) - - mrs, closed_by_mrs = service.execute(issue) - - expect(mrs).to match_array([]) - expect(closed_by_mrs).to match_array([mr, other_mr]) - end - end -end diff --git a/spec/services/issues/referenced_merge_requests_service_spec.rb b/spec/services/issues/referenced_merge_requests_service_spec.rb new file mode 100644 index 00000000000..a10893e502f --- /dev/null +++ b/spec/services/issues/referenced_merge_requests_service_spec.rb @@ -0,0 +1,72 @@ +# frozen_string_literal: true + +require 'spec_helper.rb' + +describe Issues::ReferencedMergeRequestsService do + def create_referencing_mr(attributes = {}) + create(:merge_request, attributes).tap do |merge_request| + create(:note, :system, project: project, noteable: issue, note: merge_request.to_reference(full: true)) + end + end + + def create_closing_mr(attributes = {}) + create_referencing_mr(attributes).tap do |merge_request| + create(:merge_requests_closing_issues, issue: issue, merge_request: merge_request) + end + end + + set(:user) { create(:user) } + set(:project) { create(:project, :public, :repository) } + set(:other_project) { create(:project, :public, :repository) } + set(:issue) { create(:issue, author: user, project: project) } + + set(:closing_mr) { create_closing_mr(source_project: project) } + set(:closing_mr_other_project) { create_closing_mr(source_project: other_project) } + + set(:referencing_mr) { create_referencing_mr(source_project: project, source_branch: 'csv') } + set(:referencing_mr_other_project) { create_referencing_mr(source_project: other_project, source_branch: 'csv') } + + let(:service) { described_class.new(project, user) } + + describe '#execute' do + it 'returns a list of sorted merge requests' do + mrs, closed_by_mrs = service.execute(issue) + + expect(mrs).to eq([closing_mr, referencing_mr, closing_mr_other_project, referencing_mr_other_project]) + expect(closed_by_mrs).to eq([closing_mr, closing_mr_other_project]) + end + end + + describe '#referenced_merge_requests' do + it 'returns the referenced merge requests' do + expect(service.referenced_merge_requests(issue)).to match_array([ + closing_mr, + closing_mr_other_project, + referencing_mr, + referencing_mr_other_project + ]) + end + + it 'excludes cross project references if the user cannot read cross project' do + allow(Ability).to receive(:allowed?).and_call_original + expect(Ability).to receive(:allowed?).with(user, :read_cross_project).at_least(:once).and_return(false) + + expect(service.referenced_merge_requests(issue)).not_to include(closing_mr_other_project) + expect(service.referenced_merge_requests(issue)).not_to include(referencing_mr_other_project) + end + end + + describe '#closed_by_merge_requests' do + let(:closed_issue) { build(:issue, :closed, project: project)} + + it 'returns the open merge requests that close this issue' do + create_closing_mr(source_project: project, state: 'closed') + + expect(service.closed_by_merge_requests(issue)).to match_array([closing_mr, closing_mr_other_project]) + end + + it 'returns an empty array when the current issue is closed already' do + expect(service.closed_by_merge_requests(closed_issue)).to eq([]) + end + end +end diff --git a/spec/support/helpers/cycle_analytics_helpers.rb b/spec/support/helpers/cycle_analytics_helpers.rb index c228bd2393b..e0fceae88de 100644 --- a/spec/support/helpers/cycle_analytics_helpers.rb +++ b/spec/support/helpers/cycle_analytics_helpers.rb @@ -65,7 +65,9 @@ module CycleAnalyticsHelpers end def merge_merge_requests_closing_issue(user, project, issue) - merge_requests = issue.closed_by_merge_requests(user) + merge_requests = Issues::ReferencedMergeRequestsService + .new(project, user) + .closed_by_merge_requests(issue) merge_requests.each { |merge_request| MergeRequests::MergeService.new(project, user).execute(merge_request) } end -- cgit v1.2.1 From 22d8fbacaf153c0b29738e812a22764129483eee Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Thu, 16 Aug 2018 12:08:00 +0100 Subject: Fix authors N+1 in Issues::ReferencedMergeRequestsService `#referenced_merge_requests` preloaded too many associations. Award emoji, for instance, are completely unnecessary here. `#closed_by_merge_requests` had the opposite problem: `#all_references` needs each item's author, and these weren't preloaded. --- .../referenced_merge_requests_service_spec.rb | 26 +++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) (limited to 'spec') diff --git a/spec/services/issues/referenced_merge_requests_service_spec.rb b/spec/services/issues/referenced_merge_requests_service_spec.rb index a10893e502f..0afc8c300f9 100644 --- a/spec/services/issues/referenced_merge_requests_service_spec.rb +++ b/spec/services/issues/referenced_merge_requests_service_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper.rb' describe Issues::ReferencedMergeRequestsService do def create_referencing_mr(attributes = {}) create(:merge_request, attributes).tap do |merge_request| - create(:note, :system, project: project, noteable: issue, note: merge_request.to_reference(full: true)) + create(:note, :system, project: project, noteable: issue, author: user, note: merge_request.to_reference(full: true)) end end @@ -54,6 +54,18 @@ describe Issues::ReferencedMergeRequestsService do expect(service.referenced_merge_requests(issue)).not_to include(closing_mr_other_project) expect(service.referenced_merge_requests(issue)).not_to include(referencing_mr_other_project) end + + context 'performance' do + it 'does not run a query for each note author', :use_clean_rails_memory_store_caching do + service.referenced_merge_requests(issue) # warm cache + control_count = ActiveRecord::QueryRecorder.new { service.referenced_merge_requests(issue) }.count + + create(:note, project: project, noteable: issue, author: create(:user)) + service.referenced_merge_requests(issue) # warm cache + + expect { service.referenced_merge_requests(issue) }.not_to exceed_query_limit(control_count) + end + end end describe '#closed_by_merge_requests' do @@ -68,5 +80,17 @@ describe Issues::ReferencedMergeRequestsService do it 'returns an empty array when the current issue is closed already' do expect(service.closed_by_merge_requests(closed_issue)).to eq([]) end + + context 'performance' do + it 'does not run a query for each note author', :use_clean_rails_memory_store_caching do + service.closed_by_merge_requests(issue) # warm cache + control_count = ActiveRecord::QueryRecorder.new { service.closed_by_merge_requests(issue) }.count + + create(:note, :system, project: project, noteable: issue, author: create(:user)) + service.closed_by_merge_requests(issue) # warm cache + + expect { service.closed_by_merge_requests(issue) }.not_to exceed_query_limit(control_count) + end + end end end -- cgit v1.2.1 From 2017c5c62abde0d6f24e3afc120365ee1aaaca4b Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Thu, 16 Aug 2018 12:17:41 +0100 Subject: Fix routes N+1 in Issues::ReferencedMergeRequestsService#execute Sorting here needs the project routes to be loaded, including the namespace routes. --- .../issues/referenced_merge_requests_service_spec.rb | 13 +++++++++++++ 1 file changed, 13 insertions(+) (limited to 'spec') diff --git a/spec/services/issues/referenced_merge_requests_service_spec.rb b/spec/services/issues/referenced_merge_requests_service_spec.rb index 0afc8c300f9..3349b97e688 100644 --- a/spec/services/issues/referenced_merge_requests_service_spec.rb +++ b/spec/services/issues/referenced_merge_requests_service_spec.rb @@ -35,6 +35,19 @@ describe Issues::ReferencedMergeRequestsService do expect(mrs).to eq([closing_mr, referencing_mr, closing_mr_other_project, referencing_mr_other_project]) expect(closed_by_mrs).to eq([closing_mr, closing_mr_other_project]) end + + context 'performance' do + it 'does not run extra queries when extra namespaces are included', :use_clean_rails_memory_store_caching do + service.execute(issue) # warm cache + control_count = ActiveRecord::QueryRecorder.new { service.execute(issue) }.count + + third_project = create(:project, :public) + create_closing_mr(source_project: third_project) + service.execute(issue) # warm cache + + expect { service.execute(issue) }.not_to exceed_query_limit(control_count) + end + end end describe '#referenced_merge_requests' do -- cgit v1.2.1 From fd072f7df71257e694a1d82525c4237ed9cef927 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Thu, 16 Aug 2018 13:23:02 +0100 Subject: Fix CI pipelines N+1 in Issues::ReferencedMergeRequestsService Whether the preloading belongs in the service or the controller is arguable, here. As the service is only used for one controller action, it seems reasonable to put it in the service, but that is not a definitive answer. Adding the preloads for MR project routes here doesn't seem to work, perhaps because of https://github.com/rails/rails/issues/32140. --- .../issues/referenced_merge_requests_service_spec.rb | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) (limited to 'spec') diff --git a/spec/services/issues/referenced_merge_requests_service_spec.rb b/spec/services/issues/referenced_merge_requests_service_spec.rb index 3349b97e688..d57bcf55af1 100644 --- a/spec/services/issues/referenced_merge_requests_service_spec.rb +++ b/spec/services/issues/referenced_merge_requests_service_spec.rb @@ -47,6 +47,24 @@ describe Issues::ReferencedMergeRequestsService do expect { service.execute(issue) }.not_to exceed_query_limit(control_count) end + + it 'preloads the head pipeline for each merge request, and its routes' do + # Hack to ensure no data is preserved on issue before starting the spec, + # to avoid false negatives + reloaded_issue = Issue.find(issue.id) + + pipeline_routes = lambda do |merge_requests| + merge_requests.map { |mr| mr.head_pipeline&.project&.full_path } + end + + closing_mr_other_project.update!(head_pipeline: create(:ci_pipeline)) + control_count = ActiveRecord::QueryRecorder.new { service.execute(reloaded_issue).each(&pipeline_routes) } + + closing_mr.update!(head_pipeline: create(:ci_pipeline)) + + expect { service.execute(issue).each(&pipeline_routes) } + .not_to exceed_query_limit(control_count) + end end end -- cgit v1.2.1 From b26e5546c3a523742b39a0b5b0e376367ea4c649 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Tue, 21 Aug 2018 12:56:01 +0100 Subject: Only load issue notes once when getting related MRs As we always call both methods from the controller - and elsewhere we call the more general method - and one uses all notes and the other uses system notes, then we should just load the notes and their authors once, and filter on the Ruby side. --- spec/services/issues/referenced_merge_requests_service_spec.rb | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'spec') diff --git a/spec/services/issues/referenced_merge_requests_service_spec.rb b/spec/services/issues/referenced_merge_requests_service_spec.rb index d57bcf55af1..61d1612829f 100644 --- a/spec/services/issues/referenced_merge_requests_service_spec.rb +++ b/spec/services/issues/referenced_merge_requests_service_spec.rb @@ -65,6 +65,12 @@ describe Issues::ReferencedMergeRequestsService do expect { service.execute(issue).each(&pipeline_routes) } .not_to exceed_query_limit(control_count) end + + it 'only loads issue notes once' do + expect(issue).to receive(:notes).once.and_call_original + + service.execute(issue) + end end end -- cgit v1.2.1