Support for rendering/redacting multiple documents

This commit changes the way certain documents are rendered (currently
only Notes) and how documents are redacted. Previously both rendering
and redacting would run on a per document basis. The result of this was
that for every document we'd have to run countless queries just to
figure out if we could display a set of links or not.

This commit changes things around so that redacting Markdown documents
is no longer tied into the html-pipeline Gem. This in turn allows it to
redact multiple documents in a single pass, thus reducing the number of
queries needed.

In turn rendering issue/merge request notes has been adjusted to take
advantage of this new setup. Instead of rendering Markdown somewhere
deep down in a view the Markdown is rendered and redacted in the
controller (taking the current user and all that into account). This has
been done in such a way that the "markdown()" helper method can still be
used on its own.

This particular commit also paves the way for caching rendered HTML on
object level. Right now there's an accessor method Note#note_html which
is used for setting/getting the rendered HTML. Once we cache HTML on row
level we can simply change this field to be a column and call a "save"
whenever needed and we're pretty much done.

app/controllers/projects/issues_controller.rb

@@ -62,8 +62,12 @@ class Projects::IssuesController < Projects::ApplicationController
   end
 
   def show
+    raw_notes = @issue.notes_with_associations.fresh
+
+    @notes = Banzai::NoteRenderer.
+      render(raw_notes, @project, current_user, @path, @project_wiki, @ref)
+
     @note     = @project.notes.new(noteable: @issue)
-    @notes    = @issue.notes.with_associations.fresh
     @noteable = @issue
 
     respond_to do |format|

app/controllers/projects/merge_requests_controller.rb

@@ -85,6 +85,15 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
     @grouped_diff_notes = @merge_request.notes.grouped_diff_notes
 
+    Banzai::NoteRenderer.render(
+      @grouped_diff_notes.values.flatten,
+      @project,
+      current_user,
+      @path,
+      @project_wiki,
+      @ref
+    )
+
     respond_to do |format|
       format.html
       format.json { render json: { html: view_to_html_string("projects/merge_requests/show/_diffs") } }
@@ -325,8 +334,21 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   def define_show_vars
     # Build a note object for comment form
     @note = @project.notes.new(noteable: @merge_request)
-    @discussions = @merge_request.mr_and_commit_notes.inc_author_project_award_emoji.fresh.discussions
-    @notes = @discussions.flatten
+
+    @discussions = @merge_request.mr_and_commit_notes.
+      inc_author_project_award_emoji.
+      fresh.
+      discussions
+
+    @notes = Banzai::NoteRenderer.render(
+      @discussions.flatten,
+      @project,
+      current_user,
+      @path,
+      @project_wiki,
+      @ref
+    )
+
     @noteable = @merge_request
 
     # Get commits from repository

app/controllers/projects/notes_controller.rb

@@ -24,6 +24,10 @@ class Projects::NotesController < Projects::ApplicationController
   def create
     @note = Notes::CreateService.new(project, current_user, note_params).execute
 
+    if @note.is_a?(Note)
+      Banzai::NoteRenderer.render([@note], @project, current_user)
+    end
+
     respond_to do |format|
       format.json { render json: note_json(@note) }
       format.html { redirect_back_or_default }
@@ -33,6 +37,10 @@ class Projects::NotesController < Projects::ApplicationController
   def update
     @note = Notes::UpdateService.new(project, current_user, note_params).execute(note)
 
+    if @note.is_a?(Note)
+      Banzai::NoteRenderer.render([@note], @project, current_user)
+    end
+
     respond_to do |format|
       format.json { render json: note_json(@note) }
       format.html { redirect_back_or_default }
@@ -118,6 +126,8 @@ class Projects::NotesController < Projects::ApplicationController
         name:   note.name
       }
     elsif note.valid?
+      Banzai::NoteRenderer.render([note], @project, current_user)
+
       {
         valid: true,
         id: note.id,

app/models/note.rb

@@ -6,6 +6,10 @@ class Note < ActiveRecord::Base
   include Awardable
   include Importable
 
+  # Attribute containing rendered and redacted Markdown as generated by
+  # Banzai::ObjectRenderer.
+  attr_accessor :note_html
+
   default_value_for :system, false
 
   attr_mentionable :note, pipeline: :note

app/views/projects/notes/_note.html.haml

@@ -32,7 +32,7 @@
       .note-body{class: note_editable ? 'js-task-list-container' : ''}
         .note-text
           = preserve do
-            = markdown(note.note, pipeline: :note, cache_key: [note, "note"], author: note.author)
+            = note.note_html
           = edited_time_ago_with_tooltip(note, placement: 'bottom', html_class: 'note_edited_ago', include_author: true)
         - if note_editable
           = render 'projects/notes/edit_form', note: note

config/initializers/metrics.rb

@@ -113,6 +113,10 @@ if Gitlab::Metrics.enabled?
     config.instrument_methods(Banzai::Renderer)
     config.instrument_methods(Banzai::Querying)
 
+    config.instrument_instance_methods(Banzai::ObjectRenderer)
+    config.instrument_instance_methods(Banzai::Redactor)
+    config.instrument_methods(Banzai::NoteRenderer)
+
     [Issuable, Mentionable, Participable].each do |klass|
       config.instrument_instance_methods(klass)
       config.instrument_instance_methods(klass::ClassMethods)

lib/banzai/filter/redactor_filter.rb

@@ -7,40 +7,13 @@ module Banzai
     #
     class RedactorFilter < HTML::Pipeline::Filter
       def call
-        nodes = Querying.css(doc, 'a.gfm[data-reference-type]')
-        visible = nodes_visible_to_user(nodes)
-
-        nodes.each do |node|
-          unless visible.include?(node)
-            # The reference should be replaced by the original text,
-            # which is not always the same as the rendered text.
-            text = node.attr('data-original') || node.text
-            node.replace(text)
-          end
-        end
+        Redactor.new(project, current_user).redact([doc])
 
         doc
       end
 
       private
 
-      def nodes_visible_to_user(nodes)
-        per_type = Hash.new { |h, k| h[k] = [] }
-        visible = Set.new
-
-        nodes.each do |node|
-          per_type[node.attr('data-reference-type')] << node
-        end
-
-        per_type.each do |type, nodes|
-          parser = Banzai::ReferenceParser[type].new(project, current_user)
-
-          visible.merge(parser.nodes_visible_to_user(current_user, nodes))
-        end
-
-        visible
-      end
-
       def current_user
         context[:current_user]
       end

lib/banzai/note_renderer.rb

+module Banzai
+  module NoteRenderer
+    # Renders a collection of Note instances.
+    #
+    # notes - The notes to render.
+    # project - The project to use for rendering/redacting.
+    # user - The user viewing the notes.
+    # path - The request path.
+    # wiki - The project's wiki.
+    # git_ref - The current Git reference.
+    def self.render(notes, project, user = nil, path = nil, wiki = nil, git_ref = nil)
+      renderer = ObjectRenderer.new(project,
+                                    user,
+                                    requested_path: path,
+                                    project_wiki: wiki,
+                                    ref: git_ref,
+                                    pipeline: :note)
+
+      renderer.render(notes, :note)
+    end
+  end
+end

lib/banzai/object_renderer.rb

+module Banzai
+  # Class for rendering multiple objects (e.g. Note instances) in a single pass.
+  #
+  # Rendered Markdown is stored in an attribute in every object based on the
+  # name of the attribute containing the Markdown. For example, when the
+  # attribute `note` is rendered the HTML is stored in `note_html`.
+  class ObjectRenderer
+    attr_reader :project, :user
+
+    # Make sure to set the appropriate pipeline in the `raw_context` attribute
+    # (e.g. `:note` for Note instances).
+    #
+    # project - A Project to use for rendering and redacting Markdown.
+    # user - The user viewing the Markdown/HTML documents, if any.
+    # context - A Hash containing extra attributes to use in the rendering
+    #           pipeline.
+    def initialize(project, user = nil, raw_context = {})
+      @project = project
+      @user = user
+      @raw_context = raw_context
+    end
+
+    # Renders and redacts an Array of objects.
+    #
+    # objects - The objects to render
+    # attribute - The attribute containing the raw Markdown to render.
+    #
+    # Returns the same input objects.
+    def render(objects, attribute)
+      documents = render_objects(objects, attribute)
+      redacted = redact_documents(documents)
+
+      objects.each_with_index do |object, index|
+        object.__send__("#{attribute}_html=", redacted.fetch(index))
+      end
+
+      objects
+    end
+
+    # Renders the attribute of every given object.
+    def render_objects(objects, attribute)
+      objects.map do |object|
+        render_attribute(object, attribute)
+      end
+    end
+
+    # Redacts the list of documents.
+    #
+    # Returns an Array containing the redacted documents.
+    def redact_documents(documents)
+      redactor = Redactor.new(project, user)
+
+      redactor.redact(documents).map do |document|
+        document.to_html.html_safe
+      end
+    end
+
+    # Returns a Banzai context for the given object and attribute.
+    def context_for(object, attribute)
+      context = base_context.merge(cache_key: [object, attribute])
+
+      if object.respond_to?(:author)
+        context[:author] = object.author
+      end
+
+      context
+    end
+
+    # Renders the attribute of an object.
+    #
+    # Returns a `Nokogiri::HTML::Document`.
+    def render_attribute(object, attribute)
+      context = context_for(object, attribute)
+
+      string = object.__send__(attribute)
+      html = Banzai.render(string, context)
+
+      Banzai::Pipeline[:relative_link].to_document(html, context)
+    end
+
+    def base_context
+      @base_context ||= @raw_context.merge(current_user: user, project: project)
+    end
+  end
+end

lib/banzai/pipeline/relative_link_pipeline.rb

+module Banzai
+  module Pipeline
+    class RelativeLinkPipeline < BasePipeline
+      def self.filters
+        FilterArray[
+          Filter::RelativeLinkFilter
+        ]
+      end
+    end
+  end
+end

lib/banzai/redactor.rb

+module Banzai
+  # Class for removing Markdown references a certain user is not allowed to
+  # view.
+  class Redactor
+    attr_reader :user, :project
+
+    # 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
+    end
+
+    # Redacts the references in the given Array of documents.
+    #
+    # This method modifies the given documents in-place.
+    #
+    # documents - A list of HTML documents containing references to redact.
+    #
+    # Returns the documents passed as the first argument.
+    def redact(documents)
+      nodes = documents.flat_map do |document|
+        Querying.css(document, 'a.gfm[data-reference-type]')
+      end
+
+      redact_nodes(nodes)
+
+      documents
+    end
+
+    # Redacts the given nodes
+    #
+    # nodes - An Array of HTML nodes to redact.
+    def redact_nodes(nodes)
+      visible = nodes_visible_to_user(nodes)
+
+      nodes.each do |node|
+        unless visible.include?(node)
+          # The reference should be replaced by the original text,
+          # which is not always the same as the rendered text.
+          text = node.attr('data-original') || node.text
+          node.replace(text)
+        end
+      end
+    end
+
+    # Returns the nodes visible to the current user.
+    #
+    # nodes - The input nodes to check.
+    #
+    # Returns a new Array containing the visible nodes.
+    def nodes_visible_to_user(nodes)
+      per_type = Hash.new { |h, k| h[k] = [] }
+      visible = Set.new
+
+      nodes.each do |node|
+        per_type[node.attr('data-reference-type')] << node
+      end
+
+      per_type.each do |type, nodes|
+        parser = Banzai::ReferenceParser[type].new(project, user)
+
+        visible.merge(parser.nodes_visible_to_user(user, nodes))
+      end
+
+      visible
+    end
+  end
+end

spec/lib/banzai/note_renderer_spec.rb

+require 'spec_helper'
+
+describe Banzai::NoteRenderer do
+  describe '.render' 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',
+             pipeline: :note).
+        and_call_original
+
+      expect_any_instance_of(Banzai::ObjectRenderer).
+        to receive(:render).with([note], :note)
+
+      described_class.render([note], project, user, 'foo', wiki, 'bar')
+    end
+  end
+end

spec/lib/banzai/object_renderer_spec.rb

+require 'spec_helper'
+
+describe Banzai::ObjectRenderer do
+  let(:project) { create(:empty_project) }
+  let(:user) { project.owner }
+
+  describe '#render' do
+    it 'renders and redacts an Array of objects' do
+      renderer = described_class.new(project, user)
+      object = double(:object, note: 'hello', note_html: nil)
+
+      expect(renderer).to receive(:render_objects).with([object], :note).
+        and_call_original
+
+      expect(renderer).to receive(:redact_documents).
+        with(an_instance_of(Array)).
+        and_call_original
+
+      expect(object).to receive(:note_html=).with('<p>hello</p>')
+
+      renderer.render([object], :note)
+    end
+  end
+
+  describe '#render_objects' do
+    it 'renders an Array of objects' do
+      object = double(:object, note: 'hello')
+      renderer = described_class.new(project, user)
+
+      expect(renderer).to receive(:render_attribute).with(object, :note).
+        and_call_original
+
+      rendered = renderer.render_objects([object], :note)
+
+      expect(rendered).to be_an_instance_of(Array)
+      expect(rendered[0]).to be_an_instance_of(Nokogiri::HTML::DocumentFragment)
+    end
+  end
+
+  describe '#redact_documents' do
+    it 'redacts a set of documents and returns them as an Array of Strings' do
+      doc = Nokogiri::HTML.fragment('<p>hello</p>')
+      renderer = described_class.new(project, user)
+
+      expect_any_instance_of(Banzai::Redactor).to receive(:redact).
+        with([doc]).
+        and_call_original
+
+      redacted = renderer.redact_documents([doc])
+
+      expect(redacted).to eq(['<p>hello</p>'])
+    end
+  end
+
+  describe '#context_for' do
+    let(:object) { double(:object, note: 'hello') }
+    let(:renderer) { described_class.new(project, user) }
+
+    it 'returns a Hash' do
+      expect(renderer.context_for(object, :note)).to be_an_instance_of(Hash)
+    end
+
+    it 'includes the cache key' do
+      context = renderer.context_for(object, :note)
+
+      expect(context[:cache_key]).to eq([object, :note])
+    end
+
+    context 'when the object responds to "author"' do
+      it 'includes the author in the context' do
+        expect(object).to receive(:author).and_return('Alice')
+
+        context = renderer.context_for(object, :note)
+
+        expect(context[:author]).to eq('Alice')
+      end
+    end
+
+    context 'when the object does not respond to "author"' do
+      it 'does not include the author in the context' do
+        context = renderer.context_for(object, :note)
+
+        expect(context.key?(:author)).to eq(false)
+      end
+    end
+  end
+
+  describe '#render_attribute' do
+    it 'renders the attribute of an object' do
+      object = double(:doc, note: 'hello')
+      renderer = described_class.new(project, user, pipeline: :note)
+      doc = renderer.render_attribute(object, :note)
+
+      expect(doc).to be_an_instance_of(Nokogiri::HTML::DocumentFragment)
+      expect(doc.to_html).to eq('<p>hello</p>')
+    end
+  end
+
+  describe '#base_context' do
+    let(:context) do
+      described_class.new(project, user, pipeline: :note).base_context
+    end
+
+    it 'returns a Hash' do
+      expect(context).to be_an_instance_of(Hash)
+    end
+
+    it 'includes the custom attributes' do
+      expect(context[:pipeline]).to eq(:note)
+    end
+
+    it 'includes the current user' do
+      expect(context[:current_user]).to eq(user)
+    end
+
+    it 'includes the current project' do
+      expect(context[:project]).to eq(project)
+    end
+  end
+end

spec/lib/banzai/redactor_spec.rb

+require 'spec_helper'
+
+describe Banzai::Redactor do
+  let(:user) { build(:user) }
+  let(:project) { build(:empty_project) }
+  let(:redactor) { described_class.new(project, user) }
+
+  describe '#redact' do
+    it 'redacts an Array of documents' do
+      doc1 = Nokogiri::HTML.
+        fragment('<a class="gfm" data-reference-type="issue">foo</a>')
+
+      doc2 = Nokogiri::HTML.
+        fragment('<a class="gfm" data-reference-type="issue">bar</a>')
+
+      expect(redactor).to receive(:nodes_visible_to_user).and_return([])
+
+      expect(redactor.redact([doc1, doc2])).to eq([doc1, doc2])
+
+      expect(doc1.to_html).to eq('foo')
+      expect(doc2.to_html).to eq('bar')
+    end
+  end
+
+  describe '#redact_nodes' do
+    it 'redacts an Array of nodes' do
+      doc = Nokogiri::HTML.fragment('<a href="foo">foo</a>')
+      node = doc.children[0]
+
+      expect(redactor).to receive(:nodes_visible_to_user).
+        with([node]).
+        and_return(Set.new)
+
+      redactor.redact_nodes([node])
+
+      expect(doc.to_html).to eq('foo')
+    end
+  end
+
+  describe '#nodes_visible_to_user' do
+    it 'returns a Set containing the visible nodes' do
+      doc = Nokogiri::HTML.fragment('<a data-reference-type="issue"></a>')
+      node = doc.children[0]
+
+      expect_any_instance_of(Banzai::ReferenceParser::IssueParser).
+        to receive(:nodes_visible_to_user).
+        with(user, [node]).
+        and_return([node])
+
+      expect(redactor.nodes_visible_to_user([node])).to eq(Set.new([node]))
+    end
+  end
+end