Banzai project ref- share context more aggresively

Changes `Banzai::CrossProjectReference#parent_from_ref` to return the
project in the context if the project's `full_path` matches the ref
we're looking for, as it makes no sense to go to the database to find a
Project we already have loaded.
This commit is contained in:
Mario de la Ossa 2018-10-02 20:56:51 -06:00
parent 42822a7d45
commit 7cb2755617
No known key found for this signature in database
GPG key ID: 20CA8F4C6A20761B
7 changed files with 34 additions and 10 deletions

View file

@ -116,7 +116,7 @@ class LabelsFinder < UnionFinder
end
def project?
params[:project_id].present?
params[:project].present? || params[:project_id].present?
end
def projects?
@ -139,7 +139,7 @@ class LabelsFinder < UnionFinder
return @project if defined?(@project)
if project?
@project = Project.find(params[:project_id])
@project = params[:project] || Project.find(params[:project_id])
@project = nil unless authorized_to_read_labels?(@project)
else
@project = nil

View file

@ -0,0 +1,5 @@
---
title: Banzai label ref finder - minimize SQL calls by sharing context more aggresively
merge_request: 22070
author:
type: performance

View file

@ -13,6 +13,7 @@ module Banzai
# Returns a Project, or nil if the reference can't be found
def parent_from_ref(ref)
return context[:project] || context[:group] unless ref
return context[:project] if context[:project]&.full_path == ref
Project.find_by_full_path(ref)
end

View file

@ -48,7 +48,7 @@ module Banzai
include_ancestor_groups: true,
only_group_labels: true }
else
{ project_id: parent.id,
{ project: parent,
include_ancestor_groups: true }
end

View file

@ -637,6 +637,18 @@ describe Projects::IssuesController do
project_id: project,
id: id
end
it 'avoids (most) N+1s loading labels' do
label = create(:label, project: project).to_reference
labels = create_list(:label, 10, project: project).map(&:to_reference)
issue = create(:issue, project: project, description: 'Test issue')
control_count = ActiveRecord::QueryRecorder.new { issue.update(description: [issue.description, label].join(' ')) }.count
# Follow-up to get rid of this `2 * label.count` requirement: https://gitlab.com/gitlab-org/gitlab-ce/issues/52230
expect { issue.update(description: [issue.description, labels].join(' ')) }
.not_to exceed_query_limit(control_count + 2 * labels.count)
end
end
describe 'GET #realtime_changes' do

View file

@ -1,16 +1,21 @@
require 'spec_helper'
describe Banzai::CrossProjectReference do
include described_class
let(:including_class) { Class.new.include(described_class).new }
before do
allow(including_class).to receive(:context).and_return({})
allow(including_class).to receive(:parent_from_ref).and_call_original
end
describe '#parent_from_ref' do
context 'when no project was referenced' do
it 'returns the project from context' do
project = double
allow(self).to receive(:context).and_return({ project: project })
allow(including_class).to receive(:context).and_return({ project: project })
expect(parent_from_ref(nil)).to eq project
expect(including_class.parent_from_ref(nil)).to eq project
end
end
@ -18,15 +23,15 @@ describe Banzai::CrossProjectReference do
it 'returns the group from context' do
group = double
allow(self).to receive(:context).and_return({ group: group })
allow(including_class).to receive(:context).and_return({ group: group })
expect(parent_from_ref(nil)).to eq group
expect(including_class.parent_from_ref(nil)).to eq group
end
end
context 'when referenced project does not exist' do
it 'returns nil' do
expect(parent_from_ref('invalid/reference')).to be_nil
expect(including_class.parent_from_ref('invalid/reference')).to be_nil
end
end
@ -37,7 +42,7 @@ describe Banzai::CrossProjectReference do
expect(Project).to receive(:find_by_full_path)
.with('cross/reference').and_return(project2)
expect(parent_from_ref('cross/reference')).to eq project2
expect(including_class.parent_from_ref('cross/reference')).to eq project2
end
end
end

View file

@ -60,6 +60,7 @@ describe Banzai::Filter::CommitRangeReferenceFilter do
exp = act = "See #{commit1.id.reverse}...#{commit2.id}"
allow(project.repository).to receive(:commit).with(commit1.id.reverse)
allow(project.repository).to receive(:commit).with(commit2.id)
expect(reference_filter(act).to_html).to eq exp
end