Support Markdown rendering using multiple projects

This refactors the Markdown pipeline so it supports the rendering of
multiple documents that may belong to different projects. An example of
where this happens is when displaying the event feed of a group. In this
case we retrieve events for all projects in the group. Previously we
would group events per project and render these chunks separately, but
this would result in many SQL queries being executed. By extending the
Markdown pipeline to support this out of the box we can drastically
reduce the number of SQL queries.

To achieve this we introduce a new object to the pipeline:
Banzai::RenderContext. This object simply wraps two other objects: an
optional Project instance, and an optional User instance. On its own
this wouldn't be very helpful, but a RenderContext can also be used to
associate HTML documents with specific Project instances. This work is
done in Banzai::ObjectRenderer and allows us to reuse as many queries
(and results) as possible.
This commit is contained in:
Yorick Peterse 2018-04-03 15:45:17 +02:00
parent 23fb465c75
commit daad7144ec
No known key found for this signature in database
GPG key ID: EDD30D2BEB691AC9
38 changed files with 254 additions and 98 deletions

View file

@ -41,7 +41,7 @@ module NotesActions
@note = Notes::CreateService.new(note_project, current_user, create_params).execute
if @note.is_a?(Note)
Notes::RenderService.new(current_user).execute([@note], @project)
Notes::RenderService.new(current_user).execute([@note])
end
respond_to do |format|
@ -56,7 +56,7 @@ module NotesActions
@note = Notes::UpdateService.new(project, current_user, note_params).execute(note)
if @note.is_a?(Note)
Notes::RenderService.new(current_user).execute([@note], @project)
Notes::RenderService.new(current_user).execute([@note])
end
respond_to do |format|

View file

@ -4,7 +4,7 @@ module RendersNotes
preload_noteable_for_regular_notes(notes)
preload_max_access_for_authors(notes, @project)
preload_first_time_contribution_for_authors(noteable, notes)
Notes::RenderService.new(current_user).execute(notes, @project)
Notes::RenderService.new(current_user).execute(notes)
notes
end

View file

@ -173,7 +173,9 @@ class GroupsController < Groups::ApplicationController
.new(@projects, offset: params[:offset].to_i, filter: event_filter)
.to_a
Events::RenderService.new(current_user).execute(@events, atom_request: request.format.atom?)
Events::RenderService
.new(current_user)
.execute(@events, atom_request: request.format.atom?)
end
def user_actions

View file

@ -68,7 +68,7 @@ class Projects::NotesController < Projects::ApplicationController
private
def render_json_with_notes_serializer
Notes::RenderService.new(current_user).execute([note], project)
Notes::RenderService.new(current_user).execute([note])
render json: note_serializer.represent(note)
end

View file

@ -256,7 +256,7 @@ module MarkupHelper
return '' unless html.present?
context.merge!(
current_user: (current_user if defined?(current_user)),
current_user: (current_user if defined?(current_user)),
# RelativeLinkFilter
commit: @commit,

View file

@ -1,15 +1,17 @@
module Events
class RenderService < BaseRenderer
def execute(events, atom_request: false)
events.map(&:note).compact.group_by(&:project).each do |project, notes|
render_notes(notes, project, atom_request)
end
notes = events.map(&:note).compact
render_notes(notes, atom_request)
end
private
def render_notes(notes, project, atom_request)
Notes::RenderService.new(current_user).execute(notes, project, render_options(atom_request))
def render_notes(notes, atom_request)
Notes::RenderService
.new(current_user)
.execute(notes, render_options(atom_request))
end
def render_options(atom_request)

View file

@ -3,19 +3,18 @@ module Notes
# Renders a collection of Note instances.
#
# notes - The notes to render.
# project - The project to use for redacting.
# user - The user viewing the notes.
#
# Possible options:
#
# requested_path - The request path.
# project_wiki - The project's wiki.
# ref - The current Git reference.
# only_path - flag to turn relative paths into absolute ones.
# xhtml - flag to save the html in XHTML
def execute(notes, project, **opts)
renderer = Banzai::ObjectRenderer.new(project, current_user, **opts)
renderer.render(notes, :note)
def execute(notes, options = {})
Banzai::ObjectRenderer
.new(user: current_user, redaction_context: options)
.render(notes, :note)
end
end
end

View file

@ -0,0 +1,5 @@
---
title: Support Markdown rendering using multiple projects
merge_request:
author:
type: performance

View file

@ -3,7 +3,7 @@ module Banzai
ATTRIBUTES = [:description, :title].freeze
def self.render(commits, project, user = nil)
obj_renderer = ObjectRenderer.new(project, user)
obj_renderer = ObjectRenderer.new(user: user, default_project: project)
ATTRIBUTES.each { |attr| obj_renderer.render(commits, attr) }
end

View file

@ -11,7 +11,8 @@ module Banzai
def call
return doc unless context[:issuable_state_filter_enabled]
extractor = Banzai::IssuableExtractor.new(project, current_user)
context = RenderContext.new(project, current_user)
extractor = Banzai::IssuableExtractor.new(context)
issuables = extractor.extract([doc])
issuables.each do |node, issuable|

View file

@ -7,7 +7,11 @@ module Banzai
#
class RedactorFilter < HTML::Pipeline::Filter
def call
Redactor.new(project, current_user).redact([doc]) unless context[:skip_redaction]
unless context[:skip_redaction]
context = RenderContext.new(project, current_user)
Redactor.new(context).redact([doc])
end
doc
end

View file

@ -12,11 +12,11 @@ module Banzai
[@data-reference-type="issue" or @data-reference-type="merge_request"]
).freeze
attr_reader :project, :user
attr_reader :context
def initialize(project, user)
@project = project
@user = user
# context - An instance of Banzai::RenderContext.
def initialize(context)
@context = context
end
# Returns Hash in the form { node => issuable_instance }
@ -25,8 +25,10 @@ module Banzai
document.xpath(QUERY)
end
issue_parser = Banzai::ReferenceParser::IssueParser.new(project, user)
merge_request_parser = Banzai::ReferenceParser::MergeRequestParser.new(project, user)
issue_parser = Banzai::ReferenceParser::IssueParser.new(context)
merge_request_parser =
Banzai::ReferenceParser::MergeRequestParser.new(context)
issuables_for_nodes = issue_parser.records_for_nodes(nodes).merge(
merge_request_parser.records_for_nodes(nodes)

View file

@ -13,14 +13,13 @@ module Banzai
# As an example, rendering the attribute `note` would place the unredacted
# HTML into `note_html` and the redacted HTML into `redacted_note_html`.
class ObjectRenderer
attr_reader :project, :user
attr_reader :context
# project - A Project to use for redacting Markdown.
# default_project - A default Project to use for redacting Markdown.
# user - The user viewing the Markdown/HTML documents, if any.
# redaction_context - A Hash containing extra attributes to use during redaction
def initialize(project, user = nil, redaction_context = {})
@project = project
@user = user
def initialize(default_project: nil, user: nil, redaction_context: {})
@context = RenderContext.new(default_project, user)
@redaction_context = base_context.merge(redaction_context)
end
@ -48,17 +47,21 @@ module Banzai
pipeline = HTML::Pipeline.new([])
objects.map do |object|
pipeline.to_document(Banzai.render_field(object, attribute))
document = pipeline.to_document(Banzai.render_field(object, attribute))
context.associate_document(document, object)
document
end
end
def post_process_documents(documents, objects, attribute)
# Called here to populate cache, refer to IssuableExtractor docs
IssuableExtractor.new(project, user).extract(documents)
IssuableExtractor.new(context).extract(documents)
documents.zip(objects).map do |document, object|
context = context_for(object, attribute)
Banzai::Pipeline[:post_process].to_document(document, context)
pipeline_context = context_for(document, object, attribute)
Banzai::Pipeline[:post_process].to_document(document, pipeline_context)
end
end
@ -66,20 +69,21 @@ module Banzai
#
# Returns an Array containing the redacted documents.
def redact_documents(documents)
redactor = Redactor.new(project, user)
redactor = Redactor.new(context)
redactor.redact(documents)
end
# Returns a Banzai context for the given object and attribute.
def context_for(object, attribute)
@redaction_context.merge(object.banzai_render_context(attribute))
def context_for(document, object, attribute)
@redaction_context.merge(object.banzai_render_context(attribute)).merge(
project: context.project_for_node(document)
)
end
def base_context
{
current_user: user,
project: project,
current_user: context.current_user,
skip_redaction: true
}
end

View file

@ -2,13 +2,15 @@ module Banzai
# Class for removing Markdown references a certain user is not allowed to
# view.
class Redactor
attr_reader :user, :project
attr_reader :context
# project - A Project to use for redacting links.
# user - The currently logged in user (if any).
def initialize(project, user = nil)
@project = project
@user = user
# context - An instance of `Banzai::RenderContext`.
def initialize(context)
@context = context
end
def user
context.current_user
end
# Redacts the references in the given Array of documents.
@ -70,11 +72,11 @@ module Banzai
end
def redact_cross_project_references(documents)
extractor = Banzai::IssuableExtractor.new(project, user)
extractor = Banzai::IssuableExtractor.new(context)
issuables = extractor.extract(documents)
issuables.each do |node, issuable|
next if issuable.project == project
next if issuable.project == context.project_for_node(node)
node['class'] = node['class'].gsub('has-tooltip', '')
node['title'] = nil
@ -95,7 +97,7 @@ module Banzai
end
per_type.each do |type, nodes|
parser = Banzai::ReferenceParser[type].new(project, user)
parser = Banzai::ReferenceParser[type].new(context)
visible.merge(parser.nodes_visible_to_user(user, nodes))
end

View file

@ -10,8 +10,8 @@ module Banzai
end
def references(type, project, current_user = nil)
processor = Banzai::ReferenceParser[type]
.new(project, current_user)
context = RenderContext.new(project, current_user)
processor = Banzai::ReferenceParser[type].new(context)
processor.process(html_documents)
end

View file

@ -45,9 +45,13 @@ module Banzai
@data_attribute ||= "data-#{reference_type.to_s.dasherize}"
end
def initialize(project = nil, current_user = nil)
@project = project
@current_user = current_user
# context - An instance of `Banzai::RenderContext`.
def initialize(context)
@context = context
end
def project_for_node(node)
context.project_for_node(node)
end
# Returns all the nodes containing references that the user can refer to.
@ -224,7 +228,11 @@ module Banzai
private
attr_reader :current_user, :project
attr_reader :context
def current_user
context.current_user
end
# When a feature is disabled or visible only for
# team members we should not allow team members

View file

@ -5,15 +5,10 @@ module Banzai
def nodes_visible_to_user(user, nodes)
issues = records_for_nodes(nodes)
issues_to_check = issues.values
issues_to_check, cross_project_issues = partition_issues(issues, user)
unless can?(user, :read_cross_project)
issues_to_check, cross_project_issues = issues_to_check.partition do |issue|
issue.project == project
end
end
readable_issues = Ability.issues_readable_by_user(issues_to_check, user).to_set
readable_issues =
Ability.issues_readable_by_user(issues_to_check, user).to_set
nodes.select do |node|
issue_in_node = issues[node]
@ -25,7 +20,7 @@ module Banzai
# but not the issue.
if readable_issues.include?(issue_in_node)
true
elsif cross_project_issues&.include?(issue_in_node)
elsif cross_project_issues.include?(issue_in_node)
can_read_reference?(user, issue_in_node)
else
false
@ -33,6 +28,32 @@ module Banzai
end
end
# issues - A Hash mapping HTML nodes to their corresponding Issue
# instances.
# user - The current User.
def partition_issues(issues, user)
return [issues.values, []] if can?(user, :read_cross_project)
issues_to_check = []
cross_project_issues = []
# We manually partition the data since our input is a Hash and our
# output has to be an Array of issues; not an Array of (node, issue)
# pairs.
issues.each do |node, issue|
target =
if issue.project == project_for_node(node)
issues_to_check
else
cross_project_issues
end
target << issue
end
[issues_to_check, cross_project_issues]
end
def records_for_nodes(nodes)
@issues_for_nodes ||= grouped_objects_for_nodes(
nodes,

View file

@ -58,7 +58,7 @@ module Banzai
def can_read_project_reference?(node)
node_id = node.attr('data-project').to_i
project && project.id == node_id
project_for_node(node)&.id == node_id
end
def nodes_user_can_reference(current_user, nodes)
@ -71,6 +71,7 @@ module Banzai
nodes.select do |node|
project_id = node.attr(project_attr)
user_id = node.attr(author_attr)
project = project_for_node(node)
if project && project_id && project.id == project_id.to_i
true

View file

@ -0,0 +1,32 @@
# frozen_string_literal: true
module Banzai
# Object storing the current user, project, and other details used when
# parsing Markdown references.
class RenderContext
attr_reader :current_user
# default_project - The default project to use for all documents, if any.
# current_user - The user viewing the document, if any.
def initialize(default_project = nil, current_user = nil)
@current_user = current_user
@projects = Hash.new(default_project)
end
# Associates an HTML document with a Project.
#
# document - The HTML document to map to a Project.
# object - The object that produced the HTML document.
def associate_document(document, object)
# XML nodes respond to "document" but will return a Document instance,
# even when they belong to a DocumentFragment.
document = document.document if document.fragment?
@projects[document] = object.project if object.respond_to?(:project)
end
def project_for_node(node)
@projects[node.document]
end
end
end

View file

@ -6,7 +6,10 @@ describe Banzai::CommitRenderer do
user = build(:user)
project = create(:project, :repository)
expect(Banzai::ObjectRenderer).to receive(:new).with(project, user).and_call_original
expect(Banzai::ObjectRenderer)
.to receive(:new)
.with(user: user, default_project: project)
.and_call_original
described_class::ATTRIBUTES.each do |attr|
expect_any_instance_of(Banzai::ObjectRenderer).to receive(:render).with([project.commit], attr).once.and_call_original

View file

@ -3,7 +3,7 @@ require 'spec_helper'
describe Banzai::IssuableExtractor do
let(:project) { create(:project) }
let(:user) { create(:user) }
let(:extractor) { described_class.new(project, user) }
let(:extractor) { described_class.new(Banzai::RenderContext.new(project, user)) }
let(:issue) { create(:issue, project: project) }
let(:merge_request) { create(:merge_request, source_project: project) }
let(:issue_link) do

View file

@ -3,7 +3,14 @@ require 'spec_helper'
describe Banzai::ObjectRenderer do
let(:project) { create(:project, :repository) }
let(:user) { project.owner }
let(:renderer) { described_class.new(project, user, custom_value: 'value') }
let(:renderer) do
described_class.new(
default_project: project,
user: user,
redaction_context: { custom_value: 'value' }
)
end
let(:object) { Note.new(note: 'hello', note_html: '<p dir="auto">hello</p>', cached_markdown_version: CacheMarkdownField::CACHE_VERSION) }
describe '#render' do

View file

@ -3,7 +3,7 @@ require 'spec_helper'
describe Banzai::Redactor do
let(:user) { create(:user) }
let(:project) { build(:project) }
let(:redactor) { described_class.new(project, user) }
let(:redactor) { described_class.new(Banzai::RenderContext.new(project, user)) }
describe '#redact' do
context 'when reference not visible to user' do
@ -54,7 +54,7 @@ describe Banzai::Redactor do
context 'when project is in pending delete' do
let!(:issue) { create(:issue, project: project) }
let(:redactor) { described_class.new(project, user) }
let(:redactor) { described_class.new(Banzai::RenderContext.new(project, user)) }
before do
project.update(pending_delete: true)

View file

@ -5,13 +5,14 @@ describe Banzai::ReferenceParser::BaseParser do
let(:user) { create(:user) }
let(:project) { create(:project, :public) }
let(:context) { Banzai::RenderContext.new(project, user) }
subject do
klass = Class.new(described_class) do
self.reference_type = :foo
end
klass.new(project, user)
klass.new(context)
end
describe '.reference_type=' do
@ -23,6 +24,19 @@ describe Banzai::ReferenceParser::BaseParser do
end
end
describe '#project_for_node' do
it 'returns the Project for a node' do
document = instance_double('document', fragment?: false)
project = instance_double('project')
object = instance_double('object', project: project)
node = instance_double('node', document: document)
context.associate_document(document, object)
expect(subject.project_for_node(node)).to eq(project)
end
end
describe '#nodes_visible_to_user' do
let(:link) { empty_html_link }
@ -164,7 +178,7 @@ describe Banzai::ReferenceParser::BaseParser do
self.reference_type = :test
end
instance = dummy.new(project, user)
instance = dummy.new(Banzai::RenderContext.new(project, user))
document = Nokogiri::HTML.fragment('<a class="gfm"></a><a class="gfm" data-reference-type="test"></a>')
expect(instance).to receive(:gather_references)

View file

@ -5,7 +5,7 @@ describe Banzai::ReferenceParser::CommitParser do
let(:project) { create(:project, :public) }
let(:user) { create(:user) }
subject { described_class.new(project, user) }
subject { described_class.new(Banzai::RenderContext.new(project, user)) }
let(:link) { empty_html_link }
describe '#nodes_visible_to_user' do

View file

@ -5,7 +5,7 @@ describe Banzai::ReferenceParser::CommitRangeParser do
let(:project) { create(:project, :public) }
let(:user) { create(:user) }
subject { described_class.new(project, user) }
subject { described_class.new(Banzai::RenderContext.new(project, user)) }
let(:link) { empty_html_link }
describe '#nodes_visible_to_user' do

View file

@ -5,7 +5,7 @@ describe Banzai::ReferenceParser::ExternalIssueParser do
let(:project) { create(:project, :public) }
let(:user) { create(:user) }
subject { described_class.new(project, user) }
subject { described_class.new(Banzai::RenderContext.new(project, user)) }
let(:link) { empty_html_link }
describe '#nodes_visible_to_user' do

View file

@ -7,7 +7,7 @@ describe Banzai::ReferenceParser::IssueParser do
let(:user) { create(:user) }
let(:issue) { create(:issue, project: project) }
let(:link) { empty_html_link }
subject { described_class.new(project, user) }
subject { described_class.new(Banzai::RenderContext.new(project, user)) }
describe '#nodes_visible_to_user' do
context 'when the link has a data-issue attribute' do

View file

@ -6,7 +6,7 @@ describe Banzai::ReferenceParser::LabelParser do
let(:project) { create(:project, :public) }
let(:user) { create(:user) }
let(:label) { create(:label, project: project) }
subject { described_class.new(project, user) }
subject { described_class.new(Banzai::RenderContext.new(project, user)) }
let(:link) { empty_html_link }
describe '#nodes_visible_to_user' do

View file

@ -6,7 +6,7 @@ describe Banzai::ReferenceParser::MergeRequestParser do
let(:user) { create(:user) }
let(:project) { create(:project, :public) }
let(:merge_request) { create(:merge_request, source_project: project) }
subject { described_class.new(project, user) }
subject { described_class.new(Banzai::RenderContext.new(merge_request.target_project, user)) }
let(:link) { empty_html_link }
describe '#nodes_visible_to_user' do

View file

@ -6,7 +6,7 @@ describe Banzai::ReferenceParser::MilestoneParser do
let(:project) { create(:project, :public) }
let(:user) { create(:user) }
let(:milestone) { create(:milestone, project: project) }
subject { described_class.new(project, user) }
subject { described_class.new(Banzai::RenderContext.new(project, user)) }
let(:link) { empty_html_link }
describe '#nodes_visible_to_user' do

View file

@ -9,7 +9,7 @@ describe Banzai::ReferenceParser::SnippetParser do
let(:external_user) { create(:user, :external) }
let(:project_member) { create(:user) }
subject { described_class.new(project, user) }
subject { described_class.new(Banzai::RenderContext.new(project, user)) }
let(:link) { empty_html_link }
def visible_references(snippet_visibility, user = nil)

View file

@ -6,7 +6,7 @@ describe Banzai::ReferenceParser::UserParser do
let(:group) { create(:group) }
let(:user) { create(:user) }
let(:project) { create(:project, :public, group: group, creator: user) }
subject { described_class.new(project, user) }
subject { described_class.new(Banzai::RenderContext.new(project, user)) }
let(:link) { empty_html_link }
describe '#referenced_by' do

View file

@ -0,0 +1,37 @@
# frozen_string_literal: true
require 'spec_helper'
describe Banzai::RenderContext do
let(:document) { Nokogiri::HTML.fragment('<p>hello</p>') }
describe '#project_for_node' do
it 'returns the default project if no associated project was found' do
project = instance_double('project')
context = described_class.new(project)
expect(context.project_for_node(document)).to eq(project)
end
it 'returns the associated project if one was associated explicitly' do
project = instance_double('project')
obj = instance_double('object', project: project)
context = described_class.new
context.associate_document(document, obj)
expect(context.project_for_node(document)).to eq(project)
end
it 'returns the project associated with a DocumentFragment when using a node' do
project = instance_double('project')
obj = instance_double('object', project: project)
context = described_class.new
node = document.children.first
context.associate_document(document, obj)
expect(context.project_for_node(node)).to eq(project)
end
end
end

View file

@ -9,9 +9,7 @@ describe Events::RenderService do
context 'when the request format is atom' do
it 'renders the note inside events' do
expect(Banzai::ObjectRenderer).to receive(:new)
.with(event.project, user,
only_path: false,
xhtml: true)
.with(user: user, redaction_context: { only_path: false, xhtml: true })
.and_call_original
expect_any_instance_of(Banzai::ObjectRenderer)
@ -24,7 +22,7 @@ describe Events::RenderService do
context 'when the request format is not atom' do
it 'renders the note inside events' do
expect(Banzai::ObjectRenderer).to receive(:new)
.with(event.project, user, {})
.with(user: user, redaction_context: {})
.and_call_original
expect_any_instance_of(Banzai::ObjectRenderer)

View file

@ -4,23 +4,28 @@ describe Notes::RenderService do
describe '#execute' do
it 'renders a Note' do
note = double(:note)
project = double(:project)
wiki = double(:wiki)
user = double(:user)
expect(Banzai::ObjectRenderer).to receive(:new)
.with(project, user,
requested_path: 'foo',
project_wiki: wiki,
ref: 'bar',
only_path: nil,
xhtml: false)
expect(Banzai::ObjectRenderer)
.to receive(:new)
.with(
user: user,
redaction_context: {
requested_path: 'foo',
project_wiki: wiki,
ref: 'bar',
only_path: nil,
xhtml: false
}
)
.and_call_original
expect_any_instance_of(Banzai::ObjectRenderer)
.to receive(:render).with([note], :note)
.to receive(:render)
.with([note], :note)
described_class.new(user).execute([note], project,
described_class.new(user).execute([note],
requested_path: 'foo',
project_wiki: wiki,
ref: 'bar',

View file

@ -18,6 +18,11 @@ module FilterSpecHelper
context.reverse_merge!(project: project)
end
render_context = Banzai::RenderContext
.new(context[:project], context[:current_user])
context = context.merge(render_context: render_context)
described_class.call(html, context)
end

View file

@ -5,9 +5,11 @@ module ReferenceParserHelpers
shared_examples 'no N+1 queries' do
it 'avoids N+1 queries in #nodes_visible_to_user', :request_store do
context = Banzai::RenderContext.new(project, user)
record_queries = lambda do |links|
ActiveRecord::QueryRecorder.new do
described_class.new(project, user).nodes_visible_to_user(user, links)
described_class.new(context).nodes_visible_to_user(user, links)
end
end
@ -19,9 +21,11 @@ module ReferenceParserHelpers
end
it 'avoids N+1 queries in #records_for_nodes', :request_store do
context = Banzai::RenderContext.new(project, user)
record_queries = lambda do |links|
ActiveRecord::QueryRecorder.new do
described_class.new(project, user).records_for_nodes(links)
described_class.new(context).records_for_nodes(links)
end
end