Merge branch '26585-remove-readme-view-caching' into 'master'

Remove view fragment caching for project READMEs

Closes #26585 and #29594

See merge request !8838

app/helpers/application_helper.rb

@@ -196,38 +196,6 @@ module ApplicationHelper
     end
   end
 
-  def render_markup(file_name, file_content)
-    if gitlab_markdown?(file_name)
-      Hamlit::RailsHelpers.preserve(markdown(file_content))
-    elsif asciidoc?(file_name)
-      asciidoc(file_content)
-    elsif plain?(file_name)
-      content_tag :pre, class: 'plain-readme' do
-        file_content
-      end
-    else
-      other_markup(file_name, file_content)
-    end
-  rescue RuntimeError
-    simple_format(file_content)
-  end
-
-  def plain?(filename)
-    Gitlab::MarkupHelper.plain?(filename)
-  end
-
-  def markup?(filename)
-    Gitlab::MarkupHelper.markup?(filename)
-  end
-
-  def gitlab_markdown?(filename)
-    Gitlab::MarkupHelper.gitlab_markdown?(filename)
-  end
-
-  def asciidoc?(filename)
-    Gitlab::MarkupHelper.asciidoc?(filename)
-  end
-
   def promo_host
     'about.gitlab.com'
   end

app/helpers/markup_helper.rb

@@ -1,6 +1,22 @@
 require 'nokogiri'
 
-module GitlabMarkdownHelper
+module MarkupHelper
+  def plain?(filename)
+    Gitlab::MarkupHelper.plain?(filename)
+  end
+
+  def markup?(filename)
+    Gitlab::MarkupHelper.markup?(filename)
+  end
+
+  def gitlab_markdown?(filename)
+    Gitlab::MarkupHelper.gitlab_markdown?(filename)
+  end
+
+  def asciidoc?(filename)
+    Gitlab::MarkupHelper.asciidoc?(filename)
+  end
+
   # Use this in places where you would normally use link_to(gfm(...), ...).
   #
   # It solves a problem occurring with nested links (i.e.
@@ -11,7 +27,7 @@ module GitlabMarkdownHelper
   # explicitly produce the correct linking behavior (i.e.
   # "<a>outer text </a><a>gfm ref</a><a> more outer text</a>").
   def link_to_gfm(body, url, html_options = {})
-    return "" if body.blank?
+    return '' if body.blank?
 
     context = {
       project: @project,
@@ -43,52 +59,6 @@ module GitlabMarkdownHelper
     fragment.to_html.html_safe
   end
 
-  def markdown(text, context = {})
-    return "" unless text.present?
-
-    context[:project] ||= @project
-
-    html = Banzai.render(text, context)
-    banzai_postprocess(html, context)
-  end
-
-  def markdown_field(object, field)
-    object = object.for_display if object.respond_to?(:for_display)
-    return "" unless object.present?
-
-    html = Banzai.render_field(object, field)
-    banzai_postprocess(html, object.banzai_render_context(field))
-  end
-
-  def asciidoc(text)
-    Gitlab::Asciidoc.render(
-      text,
-      project:      @project,
-      current_user: (current_user if defined?(current_user)),
-
-      # RelativeLinkFilter
-      project_wiki:   @project_wiki,
-      requested_path: @path,
-      ref:            @ref,
-      commit:         @commit
-    )
-  end
-
-  def other_markup(file_name, text)
-    Gitlab::OtherMarkup.render(
-      file_name,
-      text,
-      project:      @project,
-      current_user: (current_user if defined?(current_user)),
-
-      # RelativeLinkFilter
-      project_wiki:   @project_wiki,
-      requested_path: @path,
-      ref:            @ref,
-      commit:         @commit
-    )
-  end
-
   # Return the first line of +text+, up to +max_chars+, after parsing the line
   # as Markdown.  HTML tags in the parsed output are not counted toward the
   # +max_chars+ limit.  If the length limit falls within a tag's contents, then
@@ -99,15 +69,63 @@ module GitlabMarkdownHelper
     truncate_visible(md, max_chars || md.length) if md.present?
   end
 
+  def markdown(text, context = {})
+    return '' unless text.present?
+
+    context[:project] ||= @project
+    html = markdown_unsafe(text, context)
+    banzai_postprocess(html, context)
+  end
+
+  def markdown_field(object, field)
+    object = object.for_display if object.respond_to?(:for_display)
+    return '' unless object.present?
+
+    html = Banzai.render_field(object, field)
+    banzai_postprocess(html, object.banzai_render_context(field))
+  end
+
+  def markup(file_name, text, context = {})
+    context[:project] ||= @project
+    html = context.delete(:rendered) || markup_unsafe(file_name, text, context)
+    banzai_postprocess(html, context)
+  end
+
   def render_wiki_content(wiki_page)
-    case wiki_page.format
-    when :markdown
-      markdown(wiki_page.content, pipeline: :wiki, project_wiki: @project_wiki, page_slug: wiki_page.slug)
-    when :asciidoc
-      asciidoc(wiki_page.content)
+    text = wiki_page.content
+    return '' unless text.present?
+
+    context = { pipeline: :wiki, project: @project, project_wiki: @project_wiki, page_slug: wiki_page.slug }
+
+    html =
+      case wiki_page.format
+      when :markdown
+        markdown_unsafe(text, context)
+      when :asciidoc
+        asciidoc_unsafe(text)
+      else
+        wiki_page.formatted_content.html_safe
+      end
+
+    banzai_postprocess(html, context)
+  end
+
+  def markup_unsafe(file_name, text, context = {})
+    return '' unless text.present?
+
+    if gitlab_markdown?(file_name)
+      Hamlit::RailsHelpers.preserve(markdown_unsafe(text, context))
+    elsif asciidoc?(file_name)
+      asciidoc_unsafe(text)
+    elsif plain?(file_name)
+      content_tag :pre, class: 'plain-readme' do
+        text
+      end
     else
-      wiki_page.formatted_content.html_safe
+      other_markup_unsafe(file_name, text)
     end
+  rescue RuntimeError
+    simple_format(text)
   end
 
   # Returns the text necessary to reference `entity` across projects
@@ -183,10 +201,10 @@ module GitlabMarkdownHelper
   end
 
   def markdown_toolbar_button(options = {})
-    data = options[:data].merge({ container: "body" })
+    data = options[:data].merge({ container: 'body' })
     content_tag :button,
-      type: "button",
-      class: "toolbar-btn js-md has-tooltip hidden-xs",
+      type: 'button',
+      class: 'toolbar-btn js-md has-tooltip hidden-xs',
       tabindex: -1,
       data: data,
       title: options[:title],
@@ -195,17 +213,34 @@ module GitlabMarkdownHelper
     end
   end
 
+  def markdown_unsafe(text, context = {})
+    Banzai.render(text, context)
+  end
+
+  def asciidoc_unsafe(text)
+    Gitlab::Asciidoc.render(text)
+  end
+
+  def other_markup_unsafe(file_name, text)
+    Gitlab::OtherMarkup.render(file_name, text)
+  end
+
   # Calls Banzai.post_process with some common context options
   def banzai_postprocess(html, context = {})
+    return '' unless html.present?
+
     context.merge!(
       current_user:   (current_user if defined?(current_user)),
 
       # RelativeLinkFilter
-      requested_path: @path,
+      commit:         @commit,
       project_wiki:   @project_wiki,
-      ref:            @ref
+      ref:            @ref,
+      requested_path: @path
     )
 
     Banzai.post_process(html, context)
   end
+
+  extend self
 end

app/helpers/tree_helper.rb

@@ -12,10 +12,6 @@ module TreeHelper
     tree.html_safe
   end
 
-  def render_readme(readme)
-    render_markup(readme.name, readme.data)
-  end
-
   # Return an image icon depending on the file type and mode
   #
   # type - String type of the tree item; either 'folder' or 'file'

app/mailers/base_mailer.rb

@@ -1,6 +1,6 @@
 class BaseMailer < ActionMailer::Base
   helper ApplicationHelper
-  helper GitlabMarkdownHelper
+  helper MarkupHelper
 
   attr_accessor :current_user
   helper_method :current_user, :can?

app/models/repository.rb

@@ -17,9 +17,9 @@ class Repository
   # same name. The cache key used by those methods must also match method's
   # name.
   #
-  # For example, for entry `:readme` there's a method called `readme` which
-  # stores its data in the `readme` cache key.
-  CACHED_METHODS = %i(size commit_count readme contribution_guide
+  # For example, for entry `:commit_count` there's a method called `commit_count` which
+  # stores its data in the `commit_count` cache key.
+  CACHED_METHODS = %i(size commit_count rendered_readme contribution_guide
                       changelog license_blob license_key gitignore koding_yml
                       gitlab_ci_yml branch_names tag_names branch_count
                       tag_count avatar exists? empty? root_ref).freeze
@@ -28,7 +28,7 @@ class Repository
   # changed. This Hash maps file types (as returned by Gitlab::FileDetector) to
   # the corresponding methods to call for refreshing caches.
   METHOD_CACHES_FOR_FILE_TYPES = {
-    readme: :readme,
+    readme: :rendered_readme,
     changelog: :changelog,
     license: %i(license_blob license_key),
     contributing: :contribution_guide,
@@ -527,7 +527,11 @@ class Repository
       head.readme
     end
   end
-  cache_method :readme
+
+  def rendered_readme
+    MarkupHelper.markup_unsafe(readme.name, readme.data, project: project) if readme
+  end
+  cache_method :rendered_readme
 
   def contribution_guide
     file_on_head(:contributing)

app/views/projects/_readme.html.haml

@@ -4,8 +4,7 @@
       - if can?(current_user, :push_code, @project)
         = link_to icon('pencil'), namespace_project_edit_blob_path(@project.namespace, @project, tree_join(@repository.root_ref, readme.name)), class: 'light edit-project-readme'
     .file-content.wiki
-      = cache(readme_cache_key) do
-        = render_readme(readme)
+      = markup(readme.name, readme.data, rendered: @repository.rendered_readme)
 - else
   .row-content-block.second-block.center
     %h3.page-title

app/views/projects/blob/_markup.html.haml

@@ -1,4 +1,4 @@
 - blob.load_all_data!(@repository)
 
 .file-content.wiki
-  = render_markup(blob.name, blob.data)
+  = markup(blob.name, blob.data)

app/views/projects/blob/preview.html.haml

@@ -1,12 +1,8 @@
 .diff-file
   .diff-content
-    - if gitlab_markdown?(@blob.name)
+    - if markup?(@blob.name)
       .file-content.wiki
-        = preserve do
-          = markdown(@content)
-    - elsif markup?(@blob.name)
-      .file-content.wiki
-        = raw render_markup(@blob.name, @content)
+        = markup(@blob.name, @content)
     - else
       .file-content.code.js-syntax-highlight
         - unless @diff_lines.empty?

app/views/projects/tree/_readme.html.haml

@@ -5,4 +5,4 @@
       %strong
         = readme.name
   .file-content.wiki
-    = render_readme(readme)
+    = markup(readme.name, readme.data)

app/views/search/results/_snippet_blob.html.haml

@@ -21,7 +21,7 @@
       .file-content.wiki
         - snippet_chunks.each do |chunk|
           - unless chunk[:data].empty?
-            = render_markup(snippet.file_name, chunk[:data])
+            = markup(snippet.file_name, chunk[:data])
           - else
             .file-content.code
               .nothing-here-block Empty file

app/views/shared/snippets/_blob.html.haml

@@ -24,6 +24,6 @@
       - if gitlab_markdown?(@snippet.file_name)
         = preserve(markdown_field(@snippet, :content))
       - else
-        = render_markup(@snippet.file_name, @snippet.content)
+        = markup(@snippet.file_name, @snippet.content)
   - else
     = render 'shared/file_highlight', blob: @snippet

changelogs/unreleased/26585-remove-readme-view-caching.yml

@@ -0,0 +1,4 @@
+---
+title: 'Remove view fragment caching for project READMEs'
+merge_request: 8838
+author:

lib/gitlab/asciidoc.rb

@@ -14,28 +14,16 @@ module Gitlab
     # Public: Converts the provided Asciidoc markup into HTML.
     #
     # input         - the source text in Asciidoc format
-    # context       - a Hash with the template context:
-    #                 :commit
-    #                 :project
-    #                 :project_wiki
-    #                 :requested_path
-    #                 :ref
-    # asciidoc_opts - a Hash of options to pass to the Asciidoctor converter
     #
-    def self.render(input, context, asciidoc_opts = {})
-      asciidoc_opts.reverse_merge!(
-        safe: :secure,
-        backend: :gitlab_html5,
-        attributes: []
-      )
-      asciidoc_opts[:attributes].unshift(*DEFAULT_ADOC_ATTRS)
+    def self.render(input)
+      asciidoc_opts = { safe: :secure,
+                        backend: :gitlab_html5,
+                        attributes: DEFAULT_ADOC_ATTRS }
 
       plantuml_setup
 
       html = ::Asciidoctor.convert(input, asciidoc_opts)
 
-      html = Banzai.post_process(html, context)
-
       filter = Banzai::Filter::SanitizationFilter.new(html)
       html = filter.call.to_s
 

lib/gitlab/other_markup.rb

@@ -4,19 +4,11 @@ module Gitlab
     # Public: Converts the provided markup into HTML.
     #
     # input         - the source text in a markup format
-    # context       - a Hash with the template context:
-    #                 :commit
-    #                 :project
-    #                 :project_wiki
-    #                 :requested_path
-    #                 :ref
     #
-    def self.render(file_name, input, context)
+    def self.render(file_name, input)
       html = GitHub::Markup.render(file_name, input).
         force_encoding(input.encoding)
 
-      html = Banzai.post_process(html, context)
-
       filter = Banzai::Filter::SanitizationFilter.new(html)
       html = filter.call.to_s
 

spec/features/copy_as_gfm_spec.rb

@@ -1,7 +1,7 @@
 require 'spec_helper'
 
 describe 'Copy as GFM', feature: true, js: true do
-  include GitlabMarkdownHelper
+  include MarkupHelper
   include RepoHelpers
   include ActionView::Helpers::JavaScriptHelper
 

spec/features/markdown_spec.rb

@@ -26,7 +26,7 @@ require 'erb'
 
 describe 'GitLab Markdown', feature: true do
   include Capybara::Node::Matchers
-  include GitlabMarkdownHelper
+  include MarkupHelper
   include MarkdownMatchers
 
   # Sometimes it can be useful to see the parsed output of the Markdown document

spec/helpers/application_helper_spec.rb

@@ -239,33 +239,6 @@ describe ApplicationHelper do
     end
   end
 
-  describe 'render_markup' do
-    let(:content) { 'Noël' }
-    let(:user) { create(:user) }
-    before do
-      allow(helper).to receive(:current_user).and_return(user)
-    end
-
-    it 'preserves encoding' do
-      expect(content.encoding.name).to eq('UTF-8')
-      expect(helper.render_markup('foo.rst', content).encoding.name).to eq('UTF-8')
-    end
-
-    it "delegates to #markdown when file name corresponds to Markdown" do
-      expect(helper).to receive(:gitlab_markdown?).with('foo.md').and_return(true)
-      expect(helper).to receive(:markdown).and_return('NOEL')
-
-      expect(helper.render_markup('foo.md', content)).to eq('NOEL')
-    end
-
-    it "delegates to #asciidoc when file name corresponds to AsciiDoc" do
-      expect(helper).to receive(:asciidoc?).with('foo.adoc').and_return(true)
-      expect(helper).to receive(:asciidoc).and_return('NOEL')
-
-      expect(helper.render_markup('foo.adoc', content)).to eq('NOEL')
-    end
-  end
-
   describe '#active_when' do
     it { expect(helper.active_when(true)).to eq('active') }
     it { expect(helper.active_when(false)).to eq(nil) }

spec/helpers/markup_helper_spec.rb

@@ -1,8 +1,6 @@
 require 'spec_helper'
 
-describe GitlabMarkdownHelper do
-  include ApplicationHelper
-
+describe MarkupHelper do
   let!(:project) { create(:project, :repository) }
 
   let(:user)          { create(:user, username: 'gfm') }
@@ -128,7 +126,7 @@ describe GitlabMarkdownHelper do
     it "uses Wiki pipeline for markdown files" do
       allow(@wiki).to receive(:format).and_return(:markdown)
 
-      expect(helper).to receive(:markdown).with('wiki content', pipeline: :wiki, project_wiki: @wiki, page_slug: "nested/page")
+      expect(helper).to receive(:markdown_unsafe).with('wiki content', pipeline: :wiki, project: project, project_wiki: @wiki, page_slug: "nested/page")
 
       helper.render_wiki_content(@wiki)
     end
@@ -136,7 +134,7 @@ describe GitlabMarkdownHelper do
     it "uses Asciidoctor for asciidoc files" do
       allow(@wiki).to receive(:format).and_return(:asciidoc)
 
-      expect(helper).to receive(:asciidoc).with('wiki content')
+      expect(helper).to receive(:asciidoc_unsafe).with('wiki content')
 
       helper.render_wiki_content(@wiki)
     end
@@ -151,6 +149,29 @@ describe GitlabMarkdownHelper do
     end
   end
 
+  describe 'markup' do
+    let(:content) { 'Noël' }
+
+    it 'preserves encoding' do
+      expect(content.encoding.name).to eq('UTF-8')
+      expect(helper.markup('foo.rst', content).encoding.name).to eq('UTF-8')
+    end
+
+    it "delegates to #markdown_unsafe when file name corresponds to Markdown" do
+      expect(helper).to receive(:gitlab_markdown?).with('foo.md').and_return(true)
+      expect(helper).to receive(:markdown_unsafe).and_return('NOEL')
+
+      expect(helper.markup('foo.md', content)).to eq('NOEL')
+    end
+
+    it "delegates to #asciidoc_unsafe when file name corresponds to AsciiDoc" do
+      expect(helper).to receive(:asciidoc?).with('foo.adoc').and_return(true)
+      expect(helper).to receive(:asciidoc_unsafe).and_return('NOEL')
+
+      expect(helper.markup('foo.adoc', content)).to eq('NOEL')
+    end
+  end
+
   describe '#first_line_in_markdown' do
     it 'truncates Markdown properly' do
       text = "@#{user.username}, can you look at this?\nHello world\n"

spec/lib/gitlab/asciidoc_spec.rb

@@ -22,26 +22,9 @@ module Gitlab
         expect(Asciidoctor).to receive(:convert)
           .with(input, expected_asciidoc_opts).and_return(html)
 
-        expect( render(input, context) ).to eql html
+        expect(render(input)).to eq(html)
       end
 
-      context "with asciidoc_opts" do
-        let(:asciidoc_opts) { { safe: :safe, attributes: ['foo'] } }
-
-        it "merges the options with default ones" do
-          expected_asciidoc_opts = {
-              safe: :safe,
-              backend: :gitlab_html5,
-              attributes: described_class::DEFAULT_ADOC_ATTRS + ['foo']
-          }
-
-          expect(Asciidoctor).to receive(:convert)
-            .with(input, expected_asciidoc_opts).and_return(html)
-
-          render(input, context, asciidoc_opts)
-        end
-      end
-      
       context "XSS" do
         links = {
           'links' => {
@@ -60,7 +43,7 @@ module Gitlab
 
         links.each do |name, data|
           it "does not convert dangerous #{name} into HTML" do
-            expect(render(data[:input], context)).to eql data[:output]
+            expect(render(data[:input])).to eq(data[:output])
           end
         end
       end

spec/lib/gitlab/other_markup_spec.rb

@@ -13,7 +13,7 @@ describe Gitlab::OtherMarkup, lib: true do
     }
     links.each do |name, data|
       it "does not convert dangerous #{name} into HTML" do
-        expect(render(data[:file], data[:input], context)).to eql data[:output]
+        expect(render(data[:file], data[:input])).to eq(data[:output])
       end
     end
   end

spec/models/repository_spec.rb

@@ -1803,9 +1803,9 @@ describe Repository, models: true do
   describe '#refresh_method_caches' do
     it 'refreshes the caches of the given types' do
       expect(repository).to receive(:expire_method_caches).
-        with(%i(readme license_blob license_key))
+        with(%i(rendered_readme license_blob license_key))
 
-      expect(repository).to receive(:readme)
+      expect(repository).to receive(:rendered_readme)
       expect(repository).to receive(:license_blob)
       expect(repository).to receive(:license_key)
 

spec/services/system_note_service_spec.rb

@@ -595,7 +595,7 @@ describe SystemNoteService, services: true do
     end
 
     shared_examples 'cross project mentionable' do
-      include GitlabMarkdownHelper
+      include MarkupHelper
 
       it 'contains cross reference to new noteable' do
         expect(subject.note).to include cross_project_reference(new_project, new_noteable)