Commit 03ccf795 authored by Adam Cohen's avatar Adam Cohen Committed by Sean McGivern

Reduce number of queries on Users table

parent 6302a6ed
...@@ -163,9 +163,9 @@ module CacheMarkdownField ...@@ -163,9 +163,9 @@ module CacheMarkdownField
refs = all_references(self.author) refs = all_references(self.author)
references = {} references = {}
references[:mentioned_users_ids] = refs.mentioned_users&.pluck(:id).presence references[:mentioned_users_ids] = refs.mentioned_user_ids.presence
references[:mentioned_groups_ids] = refs.mentioned_groups&.pluck(:id).presence references[:mentioned_groups_ids] = refs.mentioned_group_ids.presence
references[:mentioned_projects_ids] = refs.mentioned_projects&.pluck(:id).presence references[:mentioned_projects_ids] = refs.mentioned_project_ids.presence
# One retry is enough as next time `model_user_mention` should return the existing mention record, # One retry is enough as next time `model_user_mention` should return the existing mention record,
# that threw the `ActiveRecord::RecordNotUnique` exception in first place. # that threw the `ActiveRecord::RecordNotUnique` exception in first place.
......
...@@ -124,7 +124,7 @@ RSpec.describe EpicIssues::CreateService do ...@@ -124,7 +124,7 @@ RSpec.describe EpicIssues::CreateService do
include_examples 'returns success' include_examples 'returns success'
it 'does not perofrm N + 1 queries' do it 'does not perform N + 1 queries' do
allow(SystemNoteService).to receive(:epic_issue) allow(SystemNoteService).to receive(:epic_issue)
allow(SystemNoteService).to receive(:issue_on_epic) allow(SystemNoteService).to receive(:issue_on_epic)
...@@ -132,9 +132,9 @@ RSpec.describe EpicIssues::CreateService do ...@@ -132,9 +132,9 @@ RSpec.describe EpicIssues::CreateService do
extractor = double extractor = double
allow(Gitlab::ReferenceExtractor).to receive(:new).and_return(extractor) allow(Gitlab::ReferenceExtractor).to receive(:new).and_return(extractor)
allow(extractor).to receive(:reset_memoized_values) allow(extractor).to receive(:reset_memoized_values)
allow(extractor).to receive(:mentioned_users) allow(extractor).to receive(:mentioned_user_ids)
allow(extractor).to receive(:mentioned_groups) allow(extractor).to receive(:mentioned_group_ids)
allow(extractor).to receive(:mentioned_projects) allow(extractor).to receive(:mentioned_project_ids)
allow(extractor).to receive(:analyze) allow(extractor).to receive(:analyze)
allow(extractor).to receive(:issues).and_return([issue]) allow(extractor).to receive(:issues).and_return([issue])
......
...@@ -11,11 +11,11 @@ module Banzai ...@@ -11,11 +11,11 @@ module Banzai
@texts_and_contexts << { text: text, context: context } @texts_and_contexts << { text: text, context: context }
end end
def references(type, project, current_user = nil) def references(type, project, current_user, ids_only: false)
context = RenderContext.new(project, current_user) context = RenderContext.new(project, current_user)
processor = Banzai::ReferenceParser[type].new(context) processor = Banzai::ReferenceParser[type].new(context)
processor.process(html_documents) processor.process(html_documents, ids_only: ids_only)
end end
def reset_memoized_values def reset_memoized_values
......
...@@ -76,9 +76,11 @@ module Banzai ...@@ -76,9 +76,11 @@ module Banzai
end end
# Returns an Array of objects referenced by any of the given HTML nodes. # Returns an Array of objects referenced by any of the given HTML nodes.
def referenced_by(nodes) def referenced_by(nodes, options = {})
ids = unique_attribute_values(nodes, self.class.data_attribute) ids = unique_attribute_values(nodes, self.class.data_attribute)
return ids if options.fetch(:ids_only, false)
if ids.empty? if ids.empty?
references_relation.none references_relation.none
else else
...@@ -194,7 +196,7 @@ module Banzai ...@@ -194,7 +196,7 @@ module Banzai
# Processes the list of HTML documents and returns an Array containing all # Processes the list of HTML documents and returns an Array containing all
# the references. # the references.
def process(documents) def process(documents, ids_only: false)
type = self.class.reference_type type = self.class.reference_type
reference_options = self.class.reference_options reference_options = self.class.reference_options
...@@ -202,17 +204,17 @@ module Banzai ...@@ -202,17 +204,17 @@ module Banzai
Querying.css(document, "a[data-reference-type='#{type}'].gfm", reference_options).to_a Querying.css(document, "a[data-reference-type='#{type}'].gfm", reference_options).to_a
end end
gather_references(nodes) gather_references(nodes, ids_only: ids_only)
end end
# Gathers the references for the given HTML nodes. Returns visible # Gathers the references for the given HTML nodes. Returns visible
# references and a list of nodes which are not visible to the user # references and a list of nodes which are not visible to the user
def gather_references(nodes) def gather_references(nodes, ids_only: false)
nodes = nodes_user_can_reference(current_user, nodes) nodes = nodes_user_can_reference(current_user, nodes)
visible = nodes_visible_to_user(current_user, nodes) visible = nodes_visible_to_user(current_user, nodes)
not_visible = nodes - visible not_visible = nodes - visible
{ visible: referenced_by(visible), not_visible: not_visible } { visible: referenced_by(visible, ids_only: ids_only), not_visible: not_visible }
end end
# Returns a Hash containing the projects for a given list of HTML nodes. # Returns a Hash containing the projects for a given list of HTML nodes.
......
...@@ -5,7 +5,7 @@ module Banzai ...@@ -5,7 +5,7 @@ module Banzai
class CommitParser < BaseParser class CommitParser < BaseParser
self.reference_type = :commit self.reference_type = :commit
def referenced_by(nodes) def referenced_by(nodes, options = {})
commit_ids = commit_ids_per_project(nodes) commit_ids = commit_ids_per_project(nodes)
projects = find_projects_for_hash_keys(commit_ids) projects = find_projects_for_hash_keys(commit_ids)
......
...@@ -5,7 +5,7 @@ module Banzai ...@@ -5,7 +5,7 @@ module Banzai
class CommitRangeParser < BaseParser class CommitRangeParser < BaseParser
self.reference_type = :commit_range self.reference_type = :commit_range
def referenced_by(nodes) def referenced_by(nodes, options = {})
range_ids = commit_range_ids_per_project(nodes) range_ids = commit_range_ids_per_project(nodes)
projects = find_projects_for_hash_keys(range_ids) projects = find_projects_for_hash_keys(range_ids)
......
...@@ -5,7 +5,7 @@ module Banzai ...@@ -5,7 +5,7 @@ module Banzai
class ExternalIssueParser < BaseParser class ExternalIssueParser < BaseParser
self.reference_type = :external_issue self.reference_type = :external_issue
def referenced_by(nodes) def referenced_by(nodes, options = {})
issue_ids = issue_ids_per_project(nodes) issue_ids = issue_ids_per_project(nodes)
projects = find_projects_for_hash_keys(issue_ids) projects = find_projects_for_hash_keys(issue_ids)
issues = [] issues = []
......
...@@ -13,7 +13,7 @@ module Banzai ...@@ -13,7 +13,7 @@ module Banzai
end end
end end
def referenced_by(nodes) def referenced_by(nodes, options = {})
records = records_for_nodes(nodes) records = records_for_nodes(nodes)
nodes.map { |node| records[node] }.compact.uniq nodes.map { |node| records[node] }.compact.uniq
......
...@@ -5,7 +5,7 @@ module Banzai ...@@ -5,7 +5,7 @@ module Banzai
class UserParser < BaseParser class UserParser < BaseParser
self.reference_type = :user self.reference_type = :user
def referenced_by(nodes) def referenced_by(nodes, options = {})
group_ids = [] group_ids = []
user_ids = [] user_ids = []
project_ids = [] project_ids = []
......
...@@ -24,8 +24,8 @@ module Gitlab ...@@ -24,8 +24,8 @@ module Gitlab
super(text, context.merge(project: project)) super(text, context.merge(project: project))
end end
def references(type) def references(type, ids_only: false)
refs = super(type, project, current_user) refs = super(type, project, current_user, ids_only: ids_only)
@stateful_not_visible_counter += refs[:not_visible].count @stateful_not_visible_counter += refs[:not_visible].count
refs[:visible] refs[:visible]
...@@ -41,6 +41,12 @@ module Gitlab ...@@ -41,6 +41,12 @@ module Gitlab
define_method(type.to_s.pluralize) do define_method(type.to_s.pluralize) do
@references[type] ||= references(type) @references[type] ||= references(type)
end end
if %w(mentioned_user mentioned_group mentioned_project).include?(type.to_s)
define_method("#{type}_ids") do
@references[type] ||= references(type, ids_only: true)
end
end
end end
def issues def issues
......
...@@ -78,12 +78,31 @@ RSpec.describe Banzai::ReferenceParser::BaseParser do ...@@ -78,12 +78,31 @@ RSpec.describe Banzai::ReferenceParser::BaseParser do
describe '#referenced_by' do describe '#referenced_by' do
context 'when references_relation is implemented' do context 'when references_relation is implemented' do
it 'returns a collection of objects' do context 'and ids_only is set to false' do
links = Nokogiri::HTML.fragment("<a data-foo='#{user.id}'></a>") it 'returns a collection of objects' do
.children links = Nokogiri::HTML.fragment("<a data-foo='#{user.id}'></a>")
.children
expect(subject).to receive(:references_relation).and_return(User) expect(subject).to receive(:references_relation).and_return(User)
expect(subject.referenced_by(links)).to eq([user]) expect(subject.referenced_by(links)).to eq([user])
end
end
context 'and ids_only is set to true' do
it 'returns a collection of id values without performing a db query' do
links = Nokogiri::HTML.fragment("<a data-foo='1'></a><a data-foo='2'></a>").children
expect(subject).not_to receive(:references_relation)
expect(subject.referenced_by(links, ids_only: true)).to eq(%w(1 2))
end
context 'and the html fragment does not contain any attributes' do
it 'returns an empty array' do
links = Nokogiri::HTML.fragment("no links").children
expect(subject.referenced_by(links, ids_only: true)).to eq([])
end
end
end end
end end
...@@ -188,7 +207,7 @@ RSpec.describe Banzai::ReferenceParser::BaseParser do ...@@ -188,7 +207,7 @@ RSpec.describe Banzai::ReferenceParser::BaseParser do
dummy = Class.new(described_class) do dummy = Class.new(described_class) do
self.reference_type = :test self.reference_type = :test
def gather_references(nodes) def gather_references(nodes, ids_only: false)
nodes nodes
end end
end end
...@@ -222,7 +241,7 @@ RSpec.describe Banzai::ReferenceParser::BaseParser do ...@@ -222,7 +241,7 @@ RSpec.describe Banzai::ReferenceParser::BaseParser do
nodes.select { |n| n.id > 5 } nodes.select { |n| n.id > 5 }
end end
def referenced_by(nodes) def referenced_by(nodes, ids_only: false)
nodes.map(&:id) nodes.map(&:id)
end end
end end
......
...@@ -488,6 +488,21 @@ RSpec.describe Issues::UpdateService, :mailer do ...@@ -488,6 +488,21 @@ RSpec.describe Issues::UpdateService, :mailer do
end end
end end
end end
it 'verifies the number of queries' do
update_issue(description: "- [ ] Task 1 #{user.to_reference}")
baseline = ActiveRecord::QueryRecorder.new do
update_issue(description: "- [x] Task 1 #{user.to_reference}")
end
recorded = ActiveRecord::QueryRecorder.new do
update_issue(description: "- [x] Task 1 #{user.to_reference}\n- [ ] Task 2 #{user.to_reference}")
end
expect(recorded.count).to eq(baseline.count - 1)
expect(recorded.cached_count).to eq(0)
end
end end
context 'when description changed' do context 'when description changed' do
......
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