Commit bf0331dc authored by Francisco Javier López's avatar Francisco Javier López 🤙 Committed by Douwe Maan

Resolve "DashboardController#activity.json is slow due to SQL"

parent 34a205b3
......@@ -39,7 +39,7 @@ module NotesActions
@note = Notes::CreateService.new(note_project, current_user, create_params).execute
if @note.is_a?(Note)
Banzai::NoteRenderer.render([@note], @project, current_user)
Notes::RenderService.new(current_user).execute([@note], @project)
end
respond_to do |format|
......@@ -52,7 +52,7 @@ module NotesActions
@note = Notes::UpdateService.new(project, current_user, note_params).execute(note)
if @note.is_a?(Note)
Banzai::NoteRenderer.render([@note], @project, current_user)
Notes::RenderService.new(current_user).execute([@note], @project)
end
respond_to do |format|
......
......@@ -3,7 +3,7 @@ module RendersNotes
preload_noteable_for_regular_notes(notes)
preload_max_access_for_authors(notes, @project)
preload_first_time_contribution_for_authors(noteable, notes)
Banzai::NoteRenderer.render(notes, @project, current_user)
Notes::RenderService.new(current_user).execute(notes, @project)
notes
end
......
......@@ -57,5 +57,7 @@ class Dashboard::ProjectsController < Dashboard::ApplicationController
@events = EventCollection
.new(projects, offset: params[:offset].to_i, filter: event_filter)
.to_a
Events::RenderService.new(current_user).execute(@events, atom_request: request.format.atom?)
end
end
......@@ -32,6 +32,8 @@ class DashboardController < Dashboard::ApplicationController
@events = EventCollection
.new(projects, offset: params[:offset].to_i, filter: @event_filter)
.to_a
Events::RenderService.new(current_user).execute(@events)
end
def set_show_full_reference
......
......@@ -155,6 +155,8 @@ class GroupsController < Groups::ApplicationController
@events = EventCollection
.new(@projects, offset: params[:offset].to_i, filter: event_filter)
.to_a
Events::RenderService.new(current_user).execute(@events, atom_request: request.format.atom?)
end
def user_actions
......
......@@ -300,6 +300,8 @@ class ProjectsController < Projects::ApplicationController
@events = EventCollection
.new(projects, offset: params[:offset].to_i, filter: event_filter)
.to_a
Events::RenderService.new(current_user).execute(@events, atom_request: request.format.atom?)
end
def project_params
......
......@@ -108,6 +108,8 @@ class UsersController < ApplicationController
.references(:project)
.with_associations
.limit_recent(20, params[:offset])
Events::RenderService.new(current_user).execute(@events, atom_request: request.format.atom?)
end
def load_projects
......
......@@ -172,16 +172,6 @@ module EventsHelper
end
end
def event_note(text, options = {})
text = first_line_in_markdown(text, 150, options)
sanitize(
text,
tags: %w(a img gl-emoji b pre code p span),
attributes: Rails::Html::WhiteListSanitizer.allowed_attributes + ['style', 'data-src', 'data-name', 'data-unicode-version']
)
end
def event_commit_title(message)
message ||= ''
(message.split("\n").first || "").truncate(70)
......
......@@ -69,10 +69,16 @@ module MarkupHelper
# 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
# the tag contents are truncated without removing the closing tag.
def first_line_in_markdown(text, max_chars = nil, options = {})
md = markdown(text, options).strip
def first_line_in_markdown(object, attribute, max_chars = nil, options = {})
md = markdown_field(object, attribute, options)
truncate_visible(md, max_chars || md.length) if md.present?
text = truncate_visible(md, max_chars || md.length) if md.present?
sanitize(
text,
tags: %w(a img gl-emoji b pre code p span),
attributes: Rails::Html::WhiteListSanitizer.allowed_attributes + ['style', 'data-src', 'data-name', 'data-unicode-version']
)
end
def markdown(text, context = {})
......@@ -83,15 +89,17 @@ module MarkupHelper
prepare_for_rendering(html, context)
end
def markdown_field(object, field)
def markdown_field(object, field, context = {})
object = object.for_display if object.respond_to?(:for_display)
redacted_field_html = object.try(:"redacted_#{field}_html")
return '' unless object.present?
return redacted_field_html if redacted_field_html
html = Banzai.render_field(object, field)
prepare_for_rendering(html, object.banzai_render_context(field))
html = Banzai.render_field(object, field, context)
context.reverse_merge!(object.banzai_render_context(field)) if object.respond_to?(:banzai_render_context)
prepare_for_rendering(html, context)
end
def markup(file_name, text, context = {})
......
class BaseRenderer
attr_reader :current_user
def initialize(current_user = nil)
@current_user = current_user
end
end
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
end
private
def render_notes(notes, project, atom_request)
Notes::RenderService.new(current_user).execute(notes, project, render_options(atom_request))
end
def render_options(atom_request)
return {} unless atom_request
{ only_path: false, xhtml: true }
end
end
end
module Banzai
module NoteRenderer
module Notes
class RenderService < BaseRenderer
# Renders a collection of Note instances.
#
# notes - The notes to render.
# project - The project to use for 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)
# 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)
end
......
......@@ -36,7 +36,7 @@
.todo-body
.todo-note
.md
= event_note(todo.body, project: todo.project)
= first_line_in_markdown(todo, :body, 150, project: todo.project)
- if todo.pending?
.todo-actions
......
%div{ xmlns: "http://www.w3.org/1999/xhtml" }
= markdown(note.note, pipeline: :atom, project: note.project, author: note.author)
= markdown_field(note, :note)
......@@ -10,7 +10,7 @@
.event-body
.event-note
.md
= event_note(event.target.note, project: event.project)
= first_line_in_markdown(event.target, :note, 150, project: event.project)
- note = event.target
- if note.attachment.url
- if note.attachment.image?
......
---
title: Improve DashboardController#activity.json performance
merge_request: 14985
author:
type: performance
......@@ -77,7 +77,6 @@ def instrument_classes(instrumentation)
instrumentation.instrument_instance_methods(Banzai::ObjectRenderer)
instrumentation.instrument_instance_methods(Banzai::Redactor)
instrumentation.instrument_methods(Banzai::NoteRenderer)
[Issuable, Mentionable, Participable].each do |klass|
instrumentation.instrument_instance_methods(klass)
......
......@@ -3,8 +3,8 @@ module Banzai
Renderer.render(text, context)
end
def self.render_field(object, field)
Renderer.render_field(object, field)
def self.render_field(object, field, context = {})
Renderer.render_field(object, field, context)
end
def self.cache_collection_render(texts_and_contexts)
......
require 'uri'
module Banzai
module Filter
# HTML filter that converts relative urls into absolute ones.
class AbsoluteLinkFilter < HTML::Pipeline::Filter
def call
return doc unless context[:only_path] == false
doc.search('a.gfm').each do |el|
process_link_attr el.attribute('href')
end
doc
end
protected
def process_link_attr(html_attr)
return if html_attr.blank?
return if html_attr.value.start_with?('//')
uri = URI(html_attr.value)
html_attr.value = absolute_link_attr(uri) if uri.relative?
rescue URI::Error
# noop
end
def absolute_link_attr(uri)
URI.join(Gitlab.config.gitlab.url, uri).to_s
end
end
end
end
......@@ -311,30 +311,6 @@ module Banzai
def project_refs_cache
RequestStore[:banzai_project_refs] ||= {}
end
def cached_call(request_store_key, cache_key, path: [])
if RequestStore.active?
cache = RequestStore[request_store_key] ||= Hash.new do |hash, key|
hash[key] = Hash.new { |h, k| h[k] = {} }
end
cache = cache.dig(*path) if path.any?
get_or_set_cache(cache, cache_key) { yield }
else
yield
end
end
def get_or_set_cache(cache, key)
if cache.key?(key)
cache[key]
else
value = yield
cache[key] = value if key.present?
value
end
end
end
end
end
......@@ -8,6 +8,8 @@ module Banzai
# :project (required) - Current project, ignored if reference is cross-project.
# :only_path - Generate path-only links.
class ReferenceFilter < HTML::Pipeline::Filter
include RequestStoreReferenceCache
class << self
attr_accessor :reference_type
end
......
......@@ -60,10 +60,14 @@ module Banzai
self.class.references_in(text) do |match, username|
if username == 'all' && !skip_project_check?
link_to_all(link_content: link_content)
elsif namespace = namespaces[username.downcase]
link_to_namespace(namespace, link_content: link_content) || match
else
match
cached_call(:banzai_url_for_object, match, path: [User, username.downcase]) do
if namespace = namespaces[username.downcase]
link_to_namespace(namespace, link_content: link_content) || match
else
match
end
end
end
end
end
......@@ -74,7 +78,10 @@ module Banzai
# The keys of this Hash are the namespace paths, the values the
# corresponding Namespace objects.
def namespaces
@namespaces ||= Namespace.where_full_path_in(usernames).index_by(&:full_path).transform_keys(&:downcase)
@namespaces ||= Namespace.eager_load(:owner, :route)
.where_full_path_in(usernames)
.index_by(&:full_path)
.transform_keys(&:downcase)
end
# Returns all usernames referenced in the current document.
......
......@@ -37,7 +37,7 @@ module Banzai
objects.each_with_index do |object, index|
redacted_data = redacted[index]
object.__send__("redacted_#{attribute}_html=", redacted_data[:document].to_html.html_safe) # rubocop:disable GitlabSecurity/PublicSend
object.__send__("redacted_#{attribute}_html=", redacted_data[:document].to_html(save_options).html_safe) # rubocop:disable GitlabSecurity/PublicSend
object.user_visible_reference_count = redacted_data[:visible_reference_count] if object.respond_to?(:user_visible_reference_count)
end
end
......@@ -83,5 +83,10 @@ module Banzai
skip_redaction: true
)
end
def save_options
return {} unless base_context[:xhtml]
{ save_with: Nokogiri::XML::Node::SaveOptions::AS_XHTML }
end
end
end
......@@ -3,9 +3,10 @@ module Banzai
class PostProcessPipeline < BasePipeline
def self.filters
FilterArray[
Filter::RedactorFilter,
Filter::RelativeLinkFilter,
Filter::IssuableStateFilter,
Filter::RedactorFilter
Filter::AbsoluteLinkFilter
]
end
......
......@@ -32,12 +32,9 @@ module Banzai
# Convert a Markdown-containing field on an object into an HTML-safe String
# of HTML. This method is analogous to calling render(object.field), but it
# can cache the rendered HTML in the object, rather than Redis.
#
# The context to use is managed by the object and cannot be changed.
# Use #render, passing it the field text, if a custom rendering is needed.
def self.render_field(object, field)
def self.render_field(object, field, context = {})
unless object.respond_to?(:cached_markdown_fields)
return cacheless_render_field(object, field)
return cacheless_render_field(object, field, context)
end
object.refresh_markdown_cache! unless object.cached_html_up_to_date?(field)
......@@ -46,9 +43,9 @@ module Banzai
end
# Same as +render_field+, but without consulting or updating the cache field
def self.cacheless_render_field(object, field, options = {})
def self.cacheless_render_field(object, field, context = {})
text = object.__send__(field) # rubocop:disable GitlabSecurity/PublicSend
context = object.banzai_render_context(field).merge(options)
context = context.reverse_merge(object.banzai_render_context(field)) if object.respond_to?(:banzai_render_context)
cacheless_render(text, context)
end
......
module Banzai
module RequestStoreReferenceCache
def cached_call(request_store_key, cache_key, path: [])
if RequestStore.active?
cache = RequestStore[request_store_key] ||= Hash.new do |hash, key|
hash[key] = Hash.new { |h, k| h[k] = {} }
end
cache = cache.dig(*path) if path.any?
get_or_set_cache(cache, cache_key) { yield }
else
yield
end
end
def get_or_set_cache(cache, key)
if cache.key?(key)
cache[key]
else
value = yield
cache[key] = value if key.present?
value
end
end
end
end
require 'spec_helper'
describe EventsHelper do
describe '#event_note' do
let(:user) { build(:user) }
before do
allow(helper).to receive(:current_user).and_return(user)
end
it 'displays one line of plain text without alteration' do
input = 'A short, plain note'
expect(helper.event_note(input)).to match(input)
expect(helper.event_note(input)).not_to match(/\.\.\.\z/)
end
it 'displays inline code' do
input = 'A note with `inline code`'
expected = 'A note with <code>inline code</code>'
expect(helper.event_note(input)).to match(expected)
end
it 'truncates a note with multiple paragraphs' do
input = "Paragraph 1\n\nParagraph 2"
expected = 'Paragraph 1...'
expect(helper.event_note(input)).to match(expected)
end
it 'displays the first line of a code block' do
input = "```\nCode block\nwith two lines\n```"
expected = %r{<pre.+><code><span class="line">Code block\.\.\.</span>\n</code></pre>}
expect(helper.event_note(input)).to match(expected)
end
it 'truncates a single long line of text' do
text = 'The quick brown fox jumped over the lazy dog twice' # 50 chars
input = text * 4
expected = (text * 2).sub(/.{3}/, '...')
expect(helper.event_note(input)).to match(expected)
end
it 'preserves a link href when link text is truncated' do
text = 'The quick brown fox jumped over the lazy dog' # 44 chars
input = "#{text}#{text}#{text} " # 133 chars
link_url = 'http://example.com/foo/bar/baz' # 30 chars
input << link_url
expected_link_text = 'http://example...</a>'
expect(helper.event_note(input)).to match(link_url)
expect(helper.event_note(input)).to match(expected_link_text)
end
it 'preserves code color scheme' do
input = "```ruby\ndef test\n 'hello world'\nend\n```"
expected = "\n<pre class=\"code highlight js-syntax-highlight ruby\">" \
"<code><span class=\"line\"><span class=\"k\">def</span> <span class=\"nf\">test</span>...</span>\n" \
"</code></pre>"
expect(helper.event_note(input)).to eq(expected)
end
it 'preserves data-src for lazy images' do
input = "![ImageTest](/uploads/test.png)"
image_url = "data-src=\"/uploads/test.png\""
expect(helper.event_note(input)).to match(image_url)
end
context 'labels formatting' do
let(:input) { 'this should be ~label_1' }
def format_event_note(project)
create(:label, title: 'label_1', project: project)
helper.event_note(input, { project: project })
end
it 'preserves style attribute for a label that can be accessed by current_user' do
project = create(:project, :public)
expect(format_event_note(project)).to match(/span class=.*style=.*/)
end
it 'does not style a label that can not be accessed by current_user' do
project = create(:project, :private)
expect(format_event_note(project)).to eq("<p>#{input}</p>")
end
end
end
describe '#event_commit_title' do
let(:message) { "foo & bar " + "A" * 70 + "\n" + "B" * 80 }
subject { helper.event_commit_title(message) }
......
......@@ -67,7 +67,7 @@ describe MarkupHelper do
describe 'without redacted attribute' do
it 'renders the markdown value' do
expect(Banzai).to receive(:render_field).with(commit, attribute).and_call_original
expect(Banzai).to receive(:render_field).with(commit, attribute, {}).and_call_original
helper.markdown_field(commit, attribute)
end
......@@ -252,38 +252,141 @@ describe MarkupHelper do
end
describe '#first_line_in_markdown' do
it 'truncates Markdown properly' do
text = "@#{user.username}, can you look at this?\nHello world\n"
actual = first_line_in_markdown(text, 100, project: project)
shared_examples_for 'common markdown examples' do
let(:project_base) { build(:project, :repository) }
doc = Nokogiri::HTML.parse(actual)
it 'displays inline code' do
object = create_object('Text with `inline code`')
expected = 'Text with <code>inline code</code>'
# Make sure we didn't create invalid markup
expect(doc.errors).to be_empty
expect(first_line_in_markdown(object, attribute, 100, project: project)).to match(expected)
end
# Leading user link
expect(doc.css('a').length).to eq(1)
expect(doc.css('a')[0].attr('href')).to eq user_path(user)
expect(doc.css('a')[0].text).to eq "@#{user.username}"
it 'truncates the text with multiple paragraphs' do
object = create_object("Paragraph 1\n\nParagraph 2")
expected = 'Paragraph 1...'
expect(doc.content).to eq "@#{user.username}, can you look at this?..."
end
expect(first_line_in_markdown(object, attribute, 100, project: project)).to match(expected)
end
it 'truncates Markdown with emoji properly' do
text = "foo :wink:\nbar :grinning:"
actual = first_line_in_markdown(text, 100, project: project)
it 'displays the first line of a code block' do
object = create_object("```\nCode block\nwith two lines\n```")
expected = %r{<pre.+><code><span class="line">Code block\.\.\.</span>\n</code></pre>}
doc = Nokogiri::HTML.parse(actual)
expect(first_line_in_markdown(object, attribute, 100, project: project)).to match(expected)
end
# Make sure we didn't create invalid markup
# But also account for the 2 errors caused by the unknown `gl-emoji` elements
expect(doc.errors.length).to eq(2)
it 'truncates a single long line of text' do
text = 'The quick brown fox jumped over the lazy dog twice' # 50 chars
object = create_object(text * 4)
expected = (text * 2).sub(/.{3}/, '...')
expect(first_line_in_markdown(object, attribute, 150, project: project)).to match(expected)
end
it 'preserves a link href when link text is truncated' do
text = 'The quick brown fox jumped over the lazy dog' # 44 chars
input = "#{text}#{text}#{text} " # 133 chars
link_url = 'http://example.com/foo/bar/baz' # 30 chars
input << link_url
object = create_object(input)
expected_link_text = 'http://example...</a>'
expect(first_line_in_markdown(object, attribute, 150, project: project)).to match(link_url)
expect(first_line_in_markdown(object, attribute, 150, project: project)).to match(expected_link_text)
end
it 'preserves code color scheme' do
object = create_object("```ruby\ndef test\n 'hello world'\nend\n```")
expected = "\n<pre class=\"code highlight js-syntax-highlight ruby\">" \
"<code><span class=\"line\"><span class=\"k\">def</span> <span class=\"nf\">test</span>...</span>\n" \
"</code></pre>"
expect(first_line_in_markdown(object, attribute, 150, project: project)).to eq(expected)
end
it 'preserves data-src for lazy images' do
object = create_object("![ImageTest](/uploads/test.png)")
image_url = "data-src=\".*/uploads/test.png\""
expect(first_line_in_markdown(object, attribute, 150, project: project)).to match(image_url)
end
context 'labels formatting' do
let(:label_title) { 'this should be ~label_1' }
def create_and_format_label(project)
create(:label, title: 'label_1', project: project)
object = create_object(label_title, project: project)
expect(doc.css('gl-emoji').length).to eq(2)
expect(doc.css('gl-emoji')[0].attr('data-name')).to eq 'wink'
expect(doc.css('gl-emoji')[1].attr('data-name')).to eq 'grinning'
first_line_in_markdown(object, attribute, 150, project: project)
end
expect(doc.content).to eq "foo 😉\nbar 😀"
it 'preserves style attribute for a label that can be accessed by current_user' do
project = create(:project, :public)
expect(create_and_format_label(project)).to match(/span class=.*style=.*/)
end
it 'does not style a label that can not be accessed by current_user' do
project = create(:project, :private)
expect(create_and_format_label(project)).to eq("<p>#{label_title}</p>")
end
end
it 'truncates Markdown properly' do
object = create_object("@#{user.username}, can you look at this?\nHello world\n")
actual = first_line_in_markdown(object, attribute, 100, project: project)
doc = Nokogiri::HTML.parse(actual)
# Make sure we didn't create invalid markup
expect(doc.errors).to be_empty
# Leading user link
expect(doc.css('a').length).to eq(1)
expect(doc.css('a')[0].attr('href')).to eq user_path(user)
expect(doc.css('a')[0].text).to eq "@#{user.username}"
expect(doc.content).to eq "@#{user.username}, can you look at this?..."
end
it 'truncates Markdown with emoji properly' do
object = create_object("foo :wink:\nbar :grinning:")
actual = first_line_in_markdown(object, attribute, 100, project: project)
doc = Nokogiri::HTML.parse(actual)
# Make sure we didn't create invalid markup
# But also account for the 2 errors caused by the unknown `gl-emoji` elements
expect(doc.errors.length).to eq(2)
expect(doc.css('gl-emoji').length).to eq(2)
expect(doc.css('gl-emoji')[0].attr('data-name')).to eq 'wink'