Commit 9ec3a8b0 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'rendering-markdown-multiple-projects' into 'master'

Optimise rendering of Markdown documents that belong to different projects

See merge request gitlab-org/gitlab-ce!18157
parents ef44ebf8 daad7144
......@@ -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|
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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,
......
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)
......
......@@ -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
---
title: Support Markdown rendering using multiple projects
merge_request:
author:
type: performance
......@@ -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
......
......@@ -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|
......
......@@ -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
......
......@@ -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)
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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,
......
......@@ -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
......
# 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
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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)
......
......@@ -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)
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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)
......
......@@ -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
......
# 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
......@@ -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)
......
......@@ -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',
......
......@@ -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
......
......@@ -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
......
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