diff options
| author | Robert Speicher <robert@gitlab.com> | 2016-10-07 08:52:26 +0000 |
|---|---|---|
| committer | Robert Speicher <robert@gitlab.com> | 2016-10-07 08:52:26 +0000 |
| commit | a17412f0aa9ef9c99a5c136906ea8a585326c27d (patch) | |
| tree | 4e0d7171e0e9d6f57e151341d8b57328e4376ad1 | |
| parent | c2cf1dd6c771ee3a181fa9704da593daa825c58a (diff) | |
| parent | 110e15da260f96baffb48bebb8cba796374fbb1e (diff) | |
| download | gitlab-ce-a17412f0aa9ef9c99a5c136906ea8a585326c27d.tar.gz | |
Merge branch '18337-cache-html-in-database' into 'master'
Cache rendered Markdown fields in the database
## What does this MR do?
Introduces cache fields for Markdown-containing fields in the database, and populates them.
## Why was this MR needed?
Rendering Markdown into HTML is performance-intensive. A Redis cache already exists, but this approach is expected to be more performant and reduce unnecessary cache invalidations.
## What are the relevant issue numbers?
Closes #18337
See merge request !6095
84 files changed, 849 insertions, 195 deletions
diff --git a/CHANGELOG b/CHANGELOG index 8bfc5c97820..8049894917e 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -15,6 +15,7 @@ v 8.13.0 (unreleased) - Keep refs for each deployment - Log LDAP lookup errors and don't swallow unrelated exceptions. !6103 (Markus Koller) - Add more tests for calendar contribution (ClemMakesApps) + - Cache rendered markdown in the database, rather than Redis - Avoid database queries on Banzai::ReferenceParser::BaseParser for nodes without references - Simplify Mentionable concern instance methods - Fix permission for setting an issue's due date @@ -110,6 +110,7 @@ gem 'creole', '~> 0.5.0' gem 'wikicloth', '0.8.1' gem 'asciidoctor', '~> 1.5.2' gem 'rouge', '~> 2.0' +gem 'truncato', '~> 0.7.8' # See https://groups.google.com/forum/#!topic/ruby-security-ann/aSbgDiwb24s # and https://groups.google.com/forum/#!topic/ruby-security-ann/Dy7YiKb_pMM diff --git a/Gemfile.lock b/Gemfile.lock index 96b49faf727..b98c3acf948 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -745,6 +745,9 @@ GEM tilt (2.0.5) timecop (0.8.1) timfel-krb5-auth (0.8.3) + truncato (0.7.8) + htmlentities (~> 4.3.1) + nokogiri (~> 1.6.1) turbolinks (2.5.3) coffee-rails tzinfo (1.2.2) @@ -971,6 +974,7 @@ DEPENDENCIES test_after_commit (~> 0.4.2) thin (~> 1.7.0) timecop (~> 0.8.0) + truncato (~> 0.7.8) turbolinks (~> 2.5.0) u2f (~> 0.2.1) uglifier (~> 2.7.2) diff --git a/app/controllers/admin/broadcast_messages_controller.rb b/app/controllers/admin/broadcast_messages_controller.rb index 82055006ac0..762e36ee2e9 100644 --- a/app/controllers/admin/broadcast_messages_controller.rb +++ b/app/controllers/admin/broadcast_messages_controller.rb @@ -37,7 +37,7 @@ class Admin::BroadcastMessagesController < Admin::ApplicationController end def preview - @message = broadcast_message_params[:message] + @broadcast_message = BroadcastMessage.new(broadcast_message_params) end protected diff --git a/app/helpers/appearances_helper.rb b/app/helpers/appearances_helper.rb index de13e7a1fc2..16136d02530 100644 --- a/app/helpers/appearances_helper.rb +++ b/app/helpers/appearances_helper.rb @@ -16,7 +16,7 @@ module AppearancesHelper end def brand_text - markdown(brand_item.description) + markdown_field(brand_item, :description) end def brand_item diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index 6de25bea654..6229384817b 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -11,18 +11,6 @@ module ApplicationSettingsHelper current_application_settings.signin_enabled? end - def extra_sign_in_text - current_application_settings.sign_in_text - end - - def after_sign_up_text - current_application_settings.after_sign_up_text - end - - def shared_runners_text - current_application_settings.shared_runners_text - end - def user_oauth_applications? current_application_settings.user_oauth_applications end diff --git a/app/helpers/broadcast_messages_helper.rb b/app/helpers/broadcast_messages_helper.rb index 43a29c96bca..eb03ced67eb 100644 --- a/app/helpers/broadcast_messages_helper.rb +++ b/app/helpers/broadcast_messages_helper.rb @@ -3,7 +3,7 @@ module BroadcastMessagesHelper return unless message.present? content_tag :div, class: 'broadcast-message', style: broadcast_message_style(message) do - icon('bullhorn') << ' ' << render_broadcast_message(message.message) + icon('bullhorn') << ' ' << render_broadcast_message(message) end end @@ -32,7 +32,7 @@ module BroadcastMessagesHelper end end - def render_broadcast_message(message) - Banzai.render(message, pipeline: :broadcast_message).html_safe + def render_broadcast_message(broadcast_message) + Banzai.render_field(broadcast_message, :message).html_safe end end diff --git a/app/helpers/gitlab_markdown_helper.rb b/app/helpers/gitlab_markdown_helper.rb index 1a259656f31..0772d848289 100644 --- a/app/helpers/gitlab_markdown_helper.rb +++ b/app/helpers/gitlab_markdown_helper.rb @@ -13,14 +13,12 @@ module GitlabMarkdownHelper def link_to_gfm(body, url, html_options = {}) return "" if body.blank? - escaped_body = if body.start_with?('<img') - body - else - escape_once(body) - end - - user = current_user if defined?(current_user) - gfm_body = Banzai.render(escaped_body, project: @project, current_user: user, pipeline: :single_line) + context = { + project: @project, + current_user: (current_user if defined?(current_user)), + pipeline: :single_line, + } + gfm_body = Banzai.render(body, context) fragment = Nokogiri::HTML::DocumentFragment.parse(gfm_body) if fragment.children.size == 1 && fragment.children[0].name == 'a' @@ -51,17 +49,15 @@ module GitlabMarkdownHelper context[:project] ||= @project html = Banzai.render(text, context) + banzai_postprocess(html, context) + end - context.merge!( - current_user: (current_user if defined?(current_user)), - - # RelativeLinkFilter - requested_path: @path, - project_wiki: @project_wiki, - ref: @ref - ) + def markdown_field(object, field) + object = object.for_display if object.respond_to?(:for_display) + return "" unless object.present? - Banzai.post_process(html, context) + html = Banzai.render_field(object, field) + banzai_postprocess(html, object.banzai_render_context(field)) end def asciidoc(text) @@ -196,4 +192,18 @@ module GitlabMarkdownHelper icon(options[:icon]) end end + + # Calls Banzai.post_process with some common context options + def banzai_postprocess(html, context) + context.merge!( + current_user: (current_user if defined?(current_user)), + + # RelativeLinkFilter + requested_path: @path, + project_wiki: @project_wiki, + ref: @ref + ) + + Banzai.post_process(html, context) + end end diff --git a/app/helpers/search_helper.rb b/app/helpers/search_helper.rb index 8a7446b7cc7..aba3a3f9c5d 100644 --- a/app/helpers/search_helper.rb +++ b/app/helpers/search_helper.rb @@ -153,8 +153,18 @@ module SearchHelper search_path(options) end - # Sanitize html generated after parsing markdown from issue description or comment - def search_md_sanitize(html) + # Sanitize a HTML field for search display. Most tags are stripped out and the + # maximum length is set to 200 characters. + def search_md_sanitize(object, field) + html = markdown_field(object, field) + html = Truncato.truncate( + html, + count_tags: false, + count_tail: false, + max_length: 200 + ) + + # Truncato's filtered_tags and filtered_attributes are not quite the same sanitize(html, tags: %w(a p ol ul li pre code)) end end diff --git a/app/models/abuse_report.rb b/app/models/abuse_report.rb index b01a244032d..2340453831e 100644 --- a/app/models/abuse_report.rb +++ b/app/models/abuse_report.rb @@ -1,4 +1,8 @@ class AbuseReport < ActiveRecord::Base + include CacheMarkdownField + + cache_markdown_field :message, pipeline: :single_line + belongs_to :reporter, class_name: 'User' belongs_to :user @@ -7,6 +11,9 @@ class AbuseReport < ActiveRecord::Base validates :message, presence: true validates :user_id, uniqueness: { message: 'has already been reported' } + # For CacheMarkdownField + alias_method :author, :reporter + def remove_user(deleted_by:) user.block DeleteUserWorker.perform_async(deleted_by.id, user.id, delete_solo_owned_groups: true) diff --git a/app/models/appearance.rb b/app/models/appearance.rb index 4cf8dd9a8ce..e4106e1c2e9 100644 --- a/app/models/appearance.rb +++ b/app/models/appearance.rb @@ -1,4 +1,8 @@ class Appearance < ActiveRecord::Base + include CacheMarkdownField + + cache_markdown_field :description + validates :title, presence: true validates :description, presence: true validates :logo, file_size: { maximum: 1.megabyte } diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 55d2e07de08..c99aa7772bb 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -1,5 +1,7 @@ class ApplicationSetting < ActiveRecord::Base + include CacheMarkdownField include TokenAuthenticatable + add_authentication_token_field :runners_registration_token add_authentication_token_field :health_check_access_token @@ -17,6 +19,11 @@ class ApplicationSetting < ActiveRecord::Base serialize :domain_whitelist, Array serialize :domain_blacklist, Array + cache_markdown_field :sign_in_text + cache_markdown_field :help_page_text + cache_markdown_field :shared_runners_text, pipeline: :plain_markdown + cache_markdown_field :after_sign_up_text + attr_accessor :domain_whitelist_raw, :domain_blacklist_raw validates :session_expire_delay, diff --git a/app/models/broadcast_message.rb b/app/models/broadcast_message.rb index 61498140f27..cb40f33932a 100644 --- a/app/models/broadcast_message.rb +++ b/app/models/broadcast_message.rb @@ -1,6 +1,9 @@ class BroadcastMessage < ActiveRecord::Base + include CacheMarkdownField include Sortable + cache_markdown_field :message, pipeline: :broadcast_message + validates :message, presence: true validates :starts_at, presence: true validates :ends_at, presence: true diff --git a/app/models/concerns/cache_markdown_field.rb b/app/models/concerns/cache_markdown_field.rb new file mode 100644 index 00000000000..90bd6490a02 --- /dev/null +++ b/app/models/concerns/cache_markdown_field.rb @@ -0,0 +1,131 @@ +# This module takes care of updating cache columns for Markdown-containing +# fields. Use like this in the body of your class: +# +# include CacheMarkdownField +# cache_markdown_field :foo +# cache_markdown_field :bar +# cache_markdown_field :baz, pipeline: :single_line +# +# Corresponding foo_html, bar_html and baz_html fields should exist. +module CacheMarkdownField + # Knows about the relationship between markdown and html field names, and + # stores the rendering contexts for the latter + class FieldData + extend Forwardable + + def initialize + @data = {} + end + + def_delegators :@data, :[], :[]= + def_delegator :@data, :keys, :markdown_fields + + def html_field(markdown_field) + "#{markdown_field}_html" + end + + def html_fields + markdown_fields.map {|field| html_field(field) } + end + end + + # Dynamic registries don't really work in Rails as it's not guaranteed that + # every class will be loaded, so hardcode the list. + CACHING_CLASSES = %w[ + AbuseReport + Appearance + ApplicationSetting + BroadcastMessage + Issue + Label + MergeRequest + Milestone + Namespace + Note + Project + Release + Snippet + ] + + def self.caching_classes + CACHING_CLASSES.map(&:constantize) + end + + extend ActiveSupport::Concern + + included do + cattr_reader :cached_markdown_fields do + FieldData.new + end + + # Returns the default Banzai render context for the cached markdown field. + def banzai_render_context(field) + raise ArgumentError.new("Unknown field: #{field.inspect}") unless + cached_markdown_fields.markdown_fields.include?(field) + + # Always include a project key, or Banzai complains + project = self.project if self.respond_to?(:project) + context = cached_markdown_fields[field].merge(project: project) + + # Banzai is less strict about authors, so don't always have an author key + context[:author] = self.author if self.respond_to?(:author) + + context + end + + # Allow callers to look up the cache field name, rather than hardcoding it + def markdown_cache_field_for(field) + raise ArgumentError.new("Unknown field: #{field}") unless + cached_markdown_fields.markdown_fields.include?(field) + + cached_markdown_fields.html_field(field) + end + + # Always exclude _html fields from attributes (including serialization). + # They contain unredacted HTML, which would be a security issue + alias_method :attributes_before_markdown_cache, :attributes + def attributes + attrs = attributes_before_markdown_cache + + cached_markdown_fields.html_fields.each do |field| + attrs.delete(field) + end + + attrs + end + end + + class_methods do + private + + # Specify that a field is markdown. Its rendered output will be cached in + # a corresponding _html field. Any custom rendering options may be provided + # as a context. + def cache_markdown_field(markdown_field, context = {}) + raise "Add #{self} to CacheMarkdownField::CACHING_CLASSES" unless + CacheMarkdownField::CACHING_CLASSES.include?(self.to_s) + + cached_markdown_fields[markdown_field] = context + + html_field = cached_markdown_fields.html_field(markdown_field) + cache_method = "#{markdown_field}_cache_refresh".to_sym + invalidation_method = "#{html_field}_invalidated?".to_sym + + define_method(cache_method) do + html = Banzai::Renderer.cacheless_render_field(self, markdown_field) + __send__("#{html_field}=", html) + true + end + + # The HTML becomes invalid if any dependent fields change. For now, assume + # author and project invalidate the cache in all circumstances. + define_method(invalidation_method) do + changed_fields = changed_attributes.keys + invalidations = changed_fields & [markdown_field.to_s, "author", "project"] + !invalidations.empty? + end + + before_save cache_method, if: invalidation_method + end + end +end diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index ff465d2c745..c4b42ad82c7 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -6,6 +6,7 @@ # module Issuable extend ActiveSupport::Concern + include CacheMarkdownField include Participable include Mentionable include Subscribable @@ -13,6 +14,9 @@ module Issuable include Awardable included do + cache_markdown_field :title, pipeline: :single_line + cache_markdown_field :description + belongs_to :author, class_name: "User" belongs_to :assignee, class_name: "User" belongs_to :updated_by, class_name: "User" diff --git a/app/models/global_label.rb b/app/models/global_label.rb index ddd4bad5c21..698a7bbd327 100644 --- a/app/models/global_label.rb +++ b/app/models/global_label.rb @@ -4,6 +4,10 @@ class GlobalLabel delegate :color, :description, to: :@first_label + def for_display + @first_label + end + def self.build_collection(labels) labels = labels.group_by(&:title) diff --git a/app/models/global_milestone.rb b/app/models/global_milestone.rb index bda2b5c5d5d..cde4a568577 100644 --- a/app/models/global_milestone.rb +++ b/app/models/global_milestone.rb @@ -4,6 +4,10 @@ class GlobalMilestone attr_accessor :title, :milestones alias_attribute :name, :title + def for_display + @first_milestone + end + def self.build_collection(milestones) milestones = milestones.group_by(&:title) @@ -17,6 +21,7 @@ class GlobalMilestone @title = title @name = title @milestones = milestones + @first_milestone = milestones.find {|m| m.description.present? } || milestones.first end def safe_title diff --git a/app/models/label.rb b/app/models/label.rb index a23140b7d64..e8e12e2904e 100644 --- a/app/models/label.rb +++ b/app/models/label.rb @@ -1,4 +1,5 @@ class Label < ActiveRecord::Base + include CacheMarkdownField include Referable include Subscribable @@ -8,6 +9,8 @@ class Label < ActiveRecord::Base None = LabelStruct.new('No Label', 'No Label') Any = LabelStruct.new('Any Label', '') + cache_markdown_field :description, pipeline: :single_line + DEFAULT_COLOR = '#428BCA' default_value_for :color, DEFAULT_COLOR diff --git a/app/models/milestone.rb b/app/models/milestone.rb index 44c3cbb2c73..23aecbfa3a6 100644 --- a/app/models/milestone.rb +++ b/app/models/milestone.rb @@ -6,12 +6,16 @@ class Milestone < ActiveRecord::Base Any = MilestoneStruct.new('Any Milestone', '', -1) Upcoming = MilestoneStruct.new('Upcoming', '#upcoming', -2) + include CacheMarkdownField include InternalId include Sortable include Referable include StripAttribute include Milestoneish + cache_markdown_field :title, pipeline: :single_line + cache_markdown_field :description + belongs_to :project has_many :issues has_many :labels, -> { distinct.reorder('labels.title') }, through: :issues diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 919b3b1f095..b7f2b2bbe61 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -1,9 +1,12 @@ class Namespace < ActiveRecord::Base acts_as_paranoid + include CacheMarkdownField include Sortable include Gitlab::ShellAdapter + cache_markdown_field :description, pipeline: :description + has_many :projects, dependent: :destroy belongs_to :owner, class_name: "User" diff --git a/app/models/note.rb b/app/models/note.rb index f2656df028b..2d644b03e4d 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -6,10 +6,13 @@ class Note < ActiveRecord::Base include Awardable include Importable include FasterCacheKeys + include CacheMarkdownField + + cache_markdown_field :note, pipeline: :note # Attribute containing rendered and redacted Markdown as generated by # Banzai::ObjectRenderer. - attr_accessor :note_html + attr_accessor :redacted_note_html # An Array containing the number of visible references as generated by # Banzai::ObjectRenderer diff --git a/app/models/project.rb b/app/models/project.rb index ecd742a17d5..88e4bd14860 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -6,6 +6,7 @@ class Project < ActiveRecord::Base include Gitlab::VisibilityLevel include Gitlab::CurrentSettings include AccessRequestable + include CacheMarkdownField include Referable include Sortable include AfterCommitQueue @@ -17,6 +18,8 @@ class Project < ActiveRecord::Base UNKNOWN_IMPORT_URL = 'http://unknown.git' + cache_markdown_field :description, pipeline: :description + delegate :feature_available?, :builds_enabled?, :wiki_enabled?, :merge_requests_enabled?, to: :project_feature, allow_nil: true default_value_for :archived, false diff --git a/app/models/release.rb b/app/models/release.rb index e196b84eb18..c936899799e 100644 --- a/app/models/release.rb +++ b/app/models/release.rb @@ -1,4 +1,8 @@ class Release < ActiveRecord::Base + include CacheMarkdownField + + cache_markdown_field :description + belongs_to :project validates :description, :project, :tag, presence: true diff --git a/app/models/snippet.rb b/app/models/snippet.rb index 8a1730f3f36..2373b445009 100644 --- a/app/models/snippet.rb +++ b/app/models/snippet.rb @@ -1,11 +1,21 @@ class Snippet < ActiveRecord::Base include Gitlab::VisibilityLevel include Linguist::BlobHelper + include CacheMarkdownField include Participable include Referable include Sortable include Awardable + cache_markdown_field :title, pipeline: :single_line + cache_markdown_field :content + + # If file_name changes, it invalidates content + alias_method :default_content_html_invalidator, :content_html_invalidated? + def content_html_invalidated? + default_content_html_invalidator || file_name_changed? + end + default_value_for :visibility_level, Snippet::PRIVATE belongs_to :author, class_name: 'User' diff --git a/app/views/admin/abuse_reports/_abuse_report.html.haml b/app/views/admin/abuse_reports/_abuse_report.html.haml index 56bf6194914..05f3d9a3b50 100644 --- a/app/views/admin/abuse_reports/_abuse_report.html.haml +++ b/app/views/admin/abuse_reports/_abuse_report.html.haml @@ -21,7 +21,7 @@ %td %strong.subheading.visible-xs-block.visible-sm-block Message .message - = markdown(abuse_report.message.squish!, pipeline: :single_line, author: reporter) + = markdown_field(abuse_report, :message) %td - if user = link_to 'Remove user & report', admin_abuse_report_path(abuse_report, remove_user: true), diff --git a/app/views/admin/broadcast_messages/_form.html.haml b/app/views/admin/broadcast_messages/_form.html.haml index f952d2e9aa1..3132d157f29 100644 --- a/app/views/admin/broadcast_messages/_form.html.haml +++ b/app/views/admin/broadcast_messages/_form.html.haml @@ -1,7 +1,10 @@ .broadcast-message-preview{ style: broadcast_message_style(@broadcast_message) } = icon('bullhorn') .js-broadcast-message-preview - = render_broadcast_message(@broadcast_message.message.presence || "Your message here") + - if @broadcast_message.message.present? + = render_broadcast_message(@broadcast_message) + - else + = "Your message here" = form_for [:admin, @broadcast_message], html: { class: 'broadcast-message-form form-horizontal js-quick-submit js-requires-input'} do |f| = form_errors(@broadcast_message) diff --git a/app/views/admin/broadcast_messages/preview.js.haml b/app/views/admin/broadcast_messages/preview.js.haml index fbc9453c72e..c72e59640d7 100644 --- a/app/views/admin/broadcast_messages/preview.js.haml +++ b/app/views/admin/broadcast_messages/preview.js.haml @@ -1 +1 @@ -$('.js-broadcast-message-preview').html("#{j(render_broadcast_message(@message))}"); +$('.js-broadcast-message-preview').html("#{j(render_broadcast_message(@broadcast_message))}"); diff --git a/app/views/admin/groups/_group.html.haml b/app/views/admin/groups/_group.html.haml index 77a11e49e20..adfa1eaafc9 100644 --- a/app/views/admin/groups/_group.html.haml +++ b/app/views/admin/groups/_group.html.haml @@ -23,4 +23,4 @@ - if group.description.present? .description - = markdown(group.description, pipeline: :description) + = markdown_field(group, :description) diff --git a/app/views/admin/labels/_label.html.haml b/app/views/admin/labels/_label.html.haml index f417b2e44a4..be224d66855 100644 --- a/app/views/admin/labels/_label.html.haml +++ b/app/views/admin/labels/_label.html.haml @@ -1,7 +1,7 @@ %li{id: dom_id(label)} .label-row = render_colored_label(label, tooltip: false) - = markdown(label.description, pipeline: :single_line) + = markdown_field(label, :description) .pull-right = link_to 'Edit', edit_admin_label_path(label), class: 'btn btn-sm' = link_to 'Delete', admin_label_path(label), class: 'btn btn-sm btn-remove remove-row', method: :delete, remote: true, data: {confirm: "Delete this label? Are you sure?"} diff --git a/app/views/admin/projects/index.html.haml b/app/views/admin/projects/index.html.haml index 1e755785d90..339cfc613fe 100644 --- a/app/views/admin/projects/index.html.haml +++ b/app/views/admin/projects/index.html.haml @@ -87,7 +87,7 @@ - if project.description.present? .description - = markdown(project.description, pipeline: :description) + = markdown_field(project, :description) = paginate @projects, theme: 'gitlab' - else diff --git a/app/views/devise/confirmations/almost_there.haml b/app/views/devise/confirmations/almost_there.haml index 73c3a3dd2eb..20cd7b0179d 100644 --- a/app/views/devise/confirmations/almost_there.haml +++ b/app/views/devise/confirmations/almost_there.haml @@ -3,9 +3,9 @@ Almost there... %p.lead Please check your email to confirm your account -- if after_sign_up_text.present? +- if current_application_settings.after_sign_up_text.present? .well-confirmation.text-center - = markdown(after_sign_up_text) + = markdown_field(current_application_settings, :after_sign_up_text) %p.confirmation-content.text-center No confirmation email received? Please check your spam folder or .append-bottom-20.prepend-top-20.text-center diff --git a/app/views/groups/show.html.haml b/app/views/groups/show.html.haml index 31db6ee0cad..fab61f447c2 100644 --- a/app/views/groups/show.html.haml +++ b/app/views/groups/show.html.haml @@ -21,7 +21,7 @@ - if @group.description.present? .cover-desc.description - = markdown(@group.description, pipeline: :description) + = markdown_field(@group, :description) %div.groups-header{ class: container_class } .top-area diff --git a/app/views/help/index.html.haml b/app/views/help/index.html.haml index 57601ae9be0..31631887317 100644 --- a/app/views/help/index.html.haml +++ b/app/views/help/index.html.haml @@ -20,7 +20,7 @@ Read more about GitLab at #{link_to promo_host, promo_url, target: '_blank'}. - if current_application_settings.help_page_text.present? %hr - = markdown(current_application_settings.help_page_text) + = markdown_field(current_application_settings, :help_page_text) %hr diff --git a/app/views/layouts/devise.html.haml b/app/views/layouts/devise.html.haml index 3d28eec84ef..a9a384bd5f3 100644 --- a/app/views/layouts/devise.html.haml +++ b/app/views/layouts/devise.html.haml @@ -25,8 +25,8 @@ Perform code reviews and enhance collaboration with merge requests. Each project can also have an issue tracker and a wiki. - - if extra_sign_in_text.present? - = markdown(extra_sign_in_text) + - if current_application_settings.sign_in_text.present? + = markdown_field(current_application_settings, :sign_in_text) %hr .container diff --git a/app/views/projects/_home_panel.html.haml b/app/views/projects/_home_panel.html.haml index 8ef31ca3bda..5590198a20e 100644 --- a/app/views/projects/_home_panel.html.haml +++ b/app/views/projects/_home_panel.html.haml @@ -9,7 +9,7 @@ .project-home-desc - if @project.description.present? - = markdown(@project.description, pipeline: :description) + = markdown_field(@project, :description) - if forked_from_project = @project.forked_from_project %p diff --git a/app/views/projects/commit/_commit_box.html.haml b/app/views/projects/commit/_commit_box.html.haml index 9fd87f84aaa..6c82a4e5600 100644 --- a/app/views/projects/commit/_commit_box.html.haml +++ b/app/views/projects/commit/_commit_box.html.haml @@ -65,10 +65,10 @@ .commit-box.content-block %h3.commit-title - = markdown escape_once(@commit.title), pipeline: :single_line, author: @commit.author + = markdown(@commit.title, pipeline: :single_line, author: @commit.author) - if @commit.description.present? %pre.commit-description - = preserve(markdown(escape_once(@commit.description), pipeline: :single_line, author: @commit.author)) + = preserve(markdown(@commit.description, pipeline: :single_line, author: @commit.author)) :javascript $(".commit-info.branches").load("#{branches_namespace_project_commit_path(@project.namespace, @project, @commit.id)}"); diff --git a/app/views/projects/commits/_commit.html.haml b/app/views/projects/commits/_commit.html.haml index 389477d0927..fb48aef0559 100644 --- a/app/views/projects/commits/_commit.html.haml +++ b/app/views/projects/commits/_commit.html.haml @@ -33,7 +33,7 @@ - if commit.description? %pre.commit-row-description.js-toggle-content - = preserve(markdown(escape_once(commit.description), pipeline: :single_line, author: commit.author)) + = preserve(markdown(commit.description, pipeline: :single_line, author: commit.author)) .commit-row-info = commit_author_link(commit, avatar: false, size: 24) diff --git a/app/views/projects/issues/show.html.haml b/app/views/projects/issues/show.html.haml index cbdea209847..b94d6f8633c 100644 --- a/app/views/projects/issues/show.html.haml +++ b/app/views/projects/issues/show.html.haml @@ -55,12 +55,12 @@ .issue-details.issuable-details .detail-page-description.content-block %h2.title - = markdown escape_once(@issue.title), pipeline: :single_line, author: @issue.author + = markdown_field(@issue, :title) - if @issue.description.present? .description{ class: can?(current_user, :update_issue, @issue) ? 'js-task-list-container' : '' } .wiki = preserve do - = markdown(@issue.description, cache_key: [@issue, "description"], author: @issue.author) + = markdown_field(@issue, :description) %textarea.hidden.js-task-list-field = @issue.description = edited_time_ago_with_tooltip(@issue, placement: 'bottom', html_class: 'issue_edited_ago') diff --git a/app/views/projects/merge_requests/show/_mr_box.html.haml b/app/views/projects/merge_requests/show/_mr_box.html.haml index ebf18f6ac85..ed23d06ee5e 100644 --- a/app/views/projects/merge_requests/show/_mr_box.html.haml +++ b/app/views/projects/merge_requests/show/_mr_box.html.haml @@ -1,13 +1,13 @@ .detail-page-description.content-block %h2.title - = markdown escape_once(@merge_request.title), pipeline: :single_line, author: @merge_request.author + = markdown_field(@merge_request, :title) %div - if @merge_request.description.present? .description{class: can?(current_user, :update_merge_request, @merge_request) ? 'js-task-list-container' : ''} .wiki = preserve do - = markdown(@merge_request.description, cache_key: [@merge_request, "description"], author: @merge_request.author) + = markdown_field(@merge_request, :description) %textarea.hidden.js-task-list-field = @merge_request.description diff --git a/app/views/projects/milestones/show.html.haml b/app/views/projects/milestones/show.html.haml index 73772cc0e32..e62f810a521 100644 --- a/app/views/projects/milestones/show.html.haml +++ b/app/views/projects/milestones/show.html.haml @@ -30,13 +30,13 @@ .detail-page-description.milestone-detail %h2.title - = markdown escape_once(@milestone.title), pipeline: :single_line + = markdown_field(@milestone, :title) %div - if @milestone.description.present? .description .wiki = preserve do - = markdown @milestone.description + = markdown_field(@milestone, :description) - if @milestone.total_items_count(current_user).zero? .alert.alert-success.prepend-top-default diff --git a/app/views/projects/notes/_note.html.haml b/app/views/projects/notes/_note.html.haml index 788be4a0047..73fe6a715fa 100644 --- a/app/views/projects/notes/_note.html.haml +++ b/app/views/projects/notes/_note.html.haml @@ -61,7 +61,7 @@ .note-body{class: note_editable ? 'js-task-list-container' : ''} .note-text.md = preserve do - = note.note_html + = note.redacted_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 diff --git a/app/views/projects/pipelines/_info.html.haml b/app/views/projects/pipelines/_info.html.haml index 5800ef7de48..d288efc546f 100644 --- a/app/views/projects/pipelines/_info.html.haml +++ b/app/views/projects/pipelines/_info.html.haml @@ -33,7 +33,7 @@ - if @commit .commit-box.content-block %h3.commit-title - = markdown escape_once(@commit.title), pipeline: :single_line + = markdown(@commit.title, pipeline: :single_line) - if @commit.description.present? %pre.commit-description - = preserve(markdown(escape_once(@commit.description), pipeline: :single_line)) + = preserve(markdown(@commit.description, pipeline: :single_line)) diff --git a/app/views/projects/repositories/_feed.html.haml b/app/views/projects/repositories/_feed.html.haml index 43a6fdfd103..d9c39fb87b7 100644 --- a/app/views/projects/repositories/_feed.html.haml +++ b/app/views/projects/repositories/_feed.html.haml @@ -12,7 +12,7 @@ = link_to namespace_project_commits_path(@project.namespace, @project, commit.id) do %code= commit.short_id = image_tag avatar_icon(commit.author_email), class: "", width: 16, alt: '' - = markdown escape_once(truncate(commit.title, length: 40)), pipeline: :single_line, author: commit.author + = markdown(truncate(commit.title, length: 40), pipeline: :single_line, author: commit.author) %td %span.pull-right.cgray = time_ago_with_tooltip(commit.committed_date) diff --git a/app/views/projects/runners/_shared_runners.html.haml b/app/views/projects/runners/_shared_runners.html.haml index 752b9e060d5..5afa193357e 100644 --- a/app/views/projects/runners/_shared_runners.html.haml +++ b/app/views/projects/runners/_shared_runners.html.haml @@ -1,8 +1,8 @@ %h3 Shared Runners .bs-callout.bs-callout-warning.shared-runners-description - - if shared_runners_text.present? - = markdown(shared_runners_text, pipeline: 'plain_markdown') + - if current_application_settings.shared_runners_text.present? + = markdown_field(current_application_settings, :shared_runners_text) - else GitLab Shared Runners execute code of different projects on the same Runner unless you configure GitLab Runner Autoscale with MaxBuilds 1 (which it is diff --git a/app/views/projects/tags/_tag.html.haml b/app/views/projects/tags/_tag.html.haml index a156d98bab8..05fccb4f976 100644 --- a/app/views/projects/tags/_tag.html.haml +++ b/app/views/projects/tags/_tag.html.haml @@ -30,4 +30,4 @@ .description.prepend-top-default .wiki = preserve do - = markdown release.description + = markdown_field(release, :description) diff --git a/app/views/projects/tags/show.html.haml b/app/views/projects/tags/show.html.haml index 4dd7439b2d0..155af755759 100644 --- a/app/views/projects/tags/show.html.haml +++ b/app/views/projects/tags/show.html.haml @@ -33,6 +33,6 @@ .description .wiki = preserve do - = markdown @release.description + = markdown_field(@release, :description) - else This tag has no release notes. diff --git a/app/views/search/results/_issue.html.haml b/app/views/search/results/_issue.html.haml index 8f68d6d1b87..e010f21de5a 100644 --- a/app/views/search/results/_issue.html.haml +++ b/app/views/search/results/_issue.html.haml @@ -7,7 +7,7 @@ - if issue.description.present? .description.term = preserve do - = search_md_sanitize(markdown(truncate(issue.description, length: 200, separator: " "), { project: issue.project, author: issue.author })) + = search_md_sanitize(issue, :description) %span.light #{issue.project.name_with_namespace} - if issue.closed? diff --git a/app/views/search/results/_merge_request.html.haml b/app/views/search/results/_merge_request.html.haml index 6331c2bd6b0..07b17bc69c0 100644 --- a/app/views/search/results/_merge_request.html.haml +++ b/app/views/search/results/_merge_request.html.haml @@ -6,7 +6,7 @@ - if merge_request.description.present? .description.term = preserve do - = search_md_sanitize(markdown(merge_request.description, { project: merge_request.project, author: merge_request.author })) + = search_md_sanitize(merge_request, :description) %span.light #{merge_request.project.name_with_namespace} .pull-right diff --git a/app/views/search/results/_milestone.html.haml b/app/views/search/results/_milestone.html.haml index b31595d8d1c..9664f65a36e 100644 --- a/app/views/search/results/_milestone.html.haml +++ b/app/views/search/results/_milestone.html.haml @@ -6,4 +6,4 @@ - if milestone.description.present? .description.term = preserve do - = search_md_sanitize(markdown(milestone.description)) + = search_md_sanitize(milestone, :description) diff --git a/app/views/search/results/_note.html.haml b/app/views/search/results/_note.html.haml index e0400083870..f3701b89bb4 100644 --- a/app/views/search/results/_note.html.haml +++ b/app/views/search/results/_note.html.haml @@ -23,4 +23,4 @@ .note-search-result .term = preserve do - = search_md_sanitize(markdown(note.note, {no_header_anchors: true, author: note.author})) + = search_md_sanitize(note, :note) diff --git a/app/views/shared/_label_row.html.haml b/app/views/shared/_label_row.html.haml index 77676454b57..6f593e8dff9 100644 --- a/app/views/shared/_label_row.html.haml +++ b/app/views/shared/_label_row.html.haml @@ -12,4 +12,4 @@ = link_to_label(label, tooltip: false) - if label.description %span.label-description - = markdown(label.description, pipeline: :single_line) + = markdown_field(label, :description) diff --git a/app/views/shared/groups/_group.html.haml b/app/views/shared/groups/_group.html.haml index 1ad95351005..dc4ee3074d2 100644 --- a/app/views/shared/groups/_group.html.haml +++ b/app/views/shared/groups/_group.html.haml @@ -35,4 +35,4 @@ - if group.description.present? .description - = markdown(group.description, pipeline: :description) + = markdown_field(group, :description) diff --git a/app/views/shared/milestones/_labels_tab.html.haml b/app/views/shared/milestones/_labels_tab.html.haml index b15e8ea73fe..33f93dccd3c 100644 --- a/app/views/shared/milestones/_labels_tab.html.haml +++ b/app/views/shared/milestones/_labels_tab.html.haml @@ -8,7 +8,7 @@ = link_to milestones_label_path(options) do - render_colored_label(label, tooltip: false) %span.prepend-description-left - = markdown(label.description, pipeline: :single_line) + = markdown_field(label, :description) .pull-info-right %span.append-right-20 diff --git a/app/views/shared/milestones/_top.html.haml b/app/views/shared/milestones/_top.html.haml index 7ff947a51db..548215243db 100644 --- a/app/views/shared/milestones/_top.html.haml +++ b/app/views/shared/milestones/_top.html.haml @@ -26,7 +26,7 @@ .detail-page-description.milestone-detail %h2.title - = markdown escape_once(milestone.title), pipeline: :single_line + = markdown_field(milestone, :title) - if milestone.complete?(current_user) && milestone.active? .alert.alert-success.prepend-top-default @@ -55,4 +55,3 @@ Open %td = ms.expires_at - diff --git a/app/views/shared/projects/_project.html.haml b/app/views/shared/projects/_project.html.haml index 66c309644a7..e8668048703 100644 --- a/app/views/shared/projects/_project.html.haml +++ b/app/views/shared/projects/_project.html.haml @@ -50,4 +50,4 @@ class: "commit-row-message" - elsif project.description.present? .description - = markdown(project.description, pipeline: :description) + = markdown_field(project, :description) diff --git a/app/views/shared/snippets/_blob.html.haml b/app/views/shared/snippets/_blob.html.haml index 773ce8ac240..dcdba01aee9 100644 --- a/app/views/shared/snippets/_blob.html.haml +++ b/app/views/shared/snippets/_blob.html.haml @@ -1,9 +1,12 @@ - unless @snippet.content.empty? - if markup?(@snippet.file_name) %textarea.markdown-snippet-copy.blob-content{data: {blob_id: @snippet.id}} - = @snippet.data + = @snippet.content .file-content.wiki - = render_markup(@snippet.file_name, @snippet.data) + - if gitlab_markdown?(@snippet.file_name) + = preserve(markdown_field(@snippet, :content)) + - else + = render_markup(@snippet.file_name, @snippet.content) - else = render 'shared/file_highlight', blob: @snippet - else diff --git a/app/views/shared/snippets/_header.html.haml b/app/views/shared/snippets/_header.html.haml index 7ae4211ddfd..d7506e07ff6 100644 --- a/app/views/shared/snippets/_header.html.haml +++ b/app/views/shared/snippets/_header.html.haml @@ -21,4 +21,4 @@ = render "snippets/actions" %h2.snippet-title.prepend-top-0.append-bottom-0 - = markdown escape_once(@snippet.title), pipeline: :single_line, author: @snippet.author + = markdown_field(@snippet, :title) diff --git a/app/workers/clear_database_cache_worker.rb b/app/workers/clear_database_cache_worker.rb new file mode 100644 index 00000000000..c541daba50e --- /dev/null +++ b/app/workers/clear_database_cache_worker.rb @@ -0,0 +1,23 @@ +# This worker clears all cache fields in the database, working in batches. +class ClearDatabaseCacheWorker + include Sidekiq::Worker + + BATCH_SIZE = 1000 + + def perform + CacheMarkdownField.caching_classes.each do |kls| + fields = kls.cached_markdown_fields.html_fields + clear_cache_fields = fields.each_with_object({}) do |field, memo| + memo[field] = nil + end + + Rails.logger.debug("Clearing Markdown cache for #{kls}: #{fields.inspect}") + + kls.unscoped.in_batches(of: BATCH_SIZE) do |relation| + relation.update_all(clear_cache_fields) + end + end + + nil + end +end diff --git a/config/initializers/ar5_batching.rb b/config/initializers/ar5_batching.rb new file mode 100644 index 00000000000..35e8b3808e2 --- /dev/null +++ b/config/initializers/ar5_batching.rb @@ -0,0 +1,41 @@ +# Port ActiveRecord::Relation#in_batches from ActiveRecord 5. +# https://github.com/rails/rails/blob/ac027338e4a165273607dccee49a3d38bc836794/activerecord/lib/active_record/relation/batches.rb#L184 +# TODO: this can be removed once we're using AR5. +raise "Vendored ActiveRecord 5 code! Delete #{__FILE__}!" if ActiveRecord::VERSION::MAJOR >= 5 + +module ActiveRecord + module Batches + # Differences from upstream: enumerator support was removed, and custom + # order/limit clauses are ignored without a warning. + def in_batches(of: 1000, start: nil, finish: nil, load: false) + raise "Must provide a block" unless block_given? + + relation = self.reorder(batch_order).limit(of) + relation = relation.where(arel_table[primary_key].gteq(start)) if start + relation = relation.where(arel_table[primary_key].lteq(finish)) if finish + batch_relation = relation + + loop do + if load + records = batch_relation.records + ids = records.map(&:id) + yielded_relation = self.where(primary_key => ids) + yielded_relation.load_records(records) + else + ids = batch_relation.pluck(primary_key) + yielded_relation = self.where(primary_key => ids) + end + + break if ids.empty? + + primary_key_offset = ids.last + raise ArgumentError.new("Primary key not included in the custom select clause") unless primary_key_offset + + yield yielded_relation + + break if ids.length < of + batch_relation = relation.where(arel_table[primary_key].gt(primary_key_offset)) + end + end + end +end diff --git a/db/migrate/20160829114652_add_markdown_cache_columns.rb b/db/migrate/20160829114652_add_markdown_cache_columns.rb new file mode 100644 index 00000000000..8753e55e058 --- /dev/null +++ b/db/migrate/20160829114652_add_markdown_cache_columns.rb @@ -0,0 +1,38 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddMarkdownCacheColumns < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + COLUMNS = { + abuse_reports: [:message], + appearances: [:description], + application_settings: [ + :sign_in_text, + :help_page_text, + :shared_runners_text, + :after_sign_up_text + ], + broadcast_messages: [:message], + issues: [:title, :description], + labels: [:description], + merge_requests: [:title, :description], + milestones: [:title, :description], + namespaces: [:description], + notes: [:note], + projects: [:description], + releases: [:description], + snippets: [:title, :content], + } + + def change + COLUMNS.each do |table, columns| + columns.each do |column| + add_column table, "#{column}_html", :text + end + end + end +end diff --git a/db/schema.rb b/db/schema.rb index ad62c249b3f..56da70b3c02 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -23,6 +23,7 @@ ActiveRecord::Schema.define(version: 20160926145521) do t.text "message" t.datetime "created_at" t.datetime "updated_at" + t.text "message_html" end create_table "appearances", force: :cascade do |t| @@ -30,8 +31,9 @@ ActiveRecord::Schema.define(version: 20160926145521) do t.text "description" t.string "header_logo" t.string "logo" - t.datetime "created_at", null: false - t.datetime "updated_at", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.text "description_html" end create_table "application_settings", force: :cascade do |t| @@ -92,6 +94,10 @@ ActiveRecord::Schema.define(version: 20160926145521) do t.text "domain_blacklist" t.boolean "koding_enabled" t.string "koding_url" + t.text "sign_in_text_html" + t.text "help_page_text_html" + t.text "shared_runners_text_html" + t.text "after_sign_up_text_html" end create_table "audit_events", force: :cascade do |t| @@ -128,13 +134,14 @@ ActiveRecord::Schema.define(version: 20160926145521) do add_index "boards", ["project_id"], name: "index_boards_on_project_id", using: :btree create_table "broadcast_messages", force: :cascade do |t| - t.text "message", null: false + t.text "message", null: false t.datetime "starts_at" t.datetime "ends_at" t.datetime "created_at" t.datetime "updated_at" t.string "color" t.string "font" + t.text "message_html" end create_table "ci_application_settings", force: :cascade do |t| @@ -457,18 +464,20 @@ ActiveRecord::Schema.define(version: 20160926145521) do t.integer "project_id" t.datetime "created_at" t.datetime "updated_at" - t.integer "position", default: 0 + t.integer "position", default: 0 t.string "branch_name" t.text "description" t.integer "milestone_id" t.string "state" t.integer "iid" t.integer "updated_by_id" - t.boolean "confidential", default: false + t.boolean "confidential", default: false t.datetime "deleted_at" t.date "due_date" t.integer "moved_to_id" t.integer "lock_version" + t.text "title_html" + t.text "description_html" end add_index "issues", ["assignee_id"], name: "index_issues_on_assignee_id", using: :btree @@ -514,9 +523,10 @@ ActiveRecord::Schema.define(version: 20160926145521) do t.integer "project_id" t.datetime "created_at" t.datetime "updated_at" - t.boolean "template", default: false + t.boolean "template", default: false t.string "description" t.integer "priority" + t.text "description_html" end add_index "labels", ["priority"], name: "index_labels_on_priority", using: :btree @@ -632,6 +642,8 @@ ActiveRecord::Schema.define(version: 20160926145521) do t.datetime "deleted_at" t.string "in_progress_merge_commit_sha" t.integer "lock_version" + t.text "title_html" + t.text "description_html" end add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree @@ -658,14 +670,16 @@ ActiveRecord::Schema.define(version: 20160926145521) do add_index "merge_requests_closing_issues", ["merge_request_id"], name: "index_merge_requests_closing_issues_on_merge_request_id", using: :btree create_table "milestones", force: :cascade do |t| - t.string "title", null: false - t.integer "project_id", null: false + t.string "title", null: false + t.integer "project_id", null: false t.text "description" t.date "due_date" t.datetime "created_at" t.datetime "updated_at" t.string "state" t.integer "iid" + t.text "title_html" + t.text "description_html" end add_index "milestones", ["description"], name: "index_milestones_on_description_trigram", using: :gin, opclasses: {"description"=>"gin_trgm_ops"} @@ -689,6 +703,7 @@ ActiveRecord::Schema.define(version: 20160926145521) do t.boolean "request_access_enabled", default: true, null: false t.datetime "deleted_at" t.boolean "lfs_enabled" + t.text "description_html" end add_index "namespaces", ["created_at"], name: "index_namespaces_on_created_at", using: :btree @@ -721,6 +736,7 @@ ActiveRecord::Schema.define(version: 20160926145521) do t.integer "resolved_by_id" t.string "discussion_id" t.string "original_discussion_id" + t.text "note_html" end add_index "notes", ["author_id"], name: "index_notes_on_author_id", using: :btree @@ -872,6 +888,7 @@ ActiveRecord::Schema.define(version: 20160926145521) do t.boolean "request_access_enabled", default: true, null: false t.boolean "has_external_wiki" t.boolean "lfs_enabled" + t.text "description_html" end add_index "projects", ["ci_id"], name: "index_projects_on_ci_id", using: :btree @@ -922,6 +939,7 @@ ActiveRecord::Schema.define(version: 20160926145521) do t.integer "project_id" t.datetime "created_at" t.datetime "updated_at" + t.text "description_html" end add_index "releases", ["project_id", "tag"], name: "index_releases_on_project_id_and_tag", using: :btree @@ -976,6 +994,8 @@ ActiveRecord::Schema.define(version: 20160926145521) do t.string "file_name" t.string "type" t.integer "visibility_level", default: 0, null: false + t.text "title_html" + t.text "content_html" end add_index "snippets", ["author_id"], name: "index_snippets_on_author_id", using: :btree diff --git a/lib/banzai.rb b/lib/banzai.rb index 9ebe379f454..35ca234c1ba 100644 --- a/lib/banzai.rb +++ b/lib/banzai.rb @@ -3,6 +3,10 @@ module Banzai Renderer.render(text, context) end + def self.render_field(object, field) + Renderer.render_field(object, field) + end + def self.cache_collection_render(texts_and_contexts) Renderer.cache_collection_render(texts_and_contexts) end diff --git a/lib/banzai/filter/html_entity_filter.rb b/lib/banzai/filter/html_entity_filter.rb new file mode 100644 index 00000000000..4ef8b3b6dcf --- /dev/null +++ b/lib/banzai/filter/html_entity_filter.rb @@ -0,0 +1,12 @@ +require 'erb' + +module Banzai + module Filter + # Text filter that escapes these HTML entities: & " < > + class HTMLEntityFilter < HTML::Pipeline::TextFilter + def call + ERB::Util.html_escape(text) + end + end + end +end diff --git a/lib/banzai/note_renderer.rb b/lib/banzai/note_renderer.rb index bab6a9934d1..2b7c10f1a0e 100644 --- a/lib/banzai/note_renderer.rb +++ b/lib/banzai/note_renderer.rb @@ -3,7 +3,7 @@ module Banzai # Renders a collection of Note instances. # # notes - The notes to render. - # project - The project to use for rendering/redacting. + # project - The project to use for redacting. # user - The user viewing the notes. # path - The request path. # wiki - The project's wiki. @@ -13,8 +13,7 @@ module Banzai user, requested_path: path, project_wiki: wiki, - ref: git_ref, - pipeline: :note) + ref: git_ref) renderer.render(notes, :note) end diff --git a/lib/banzai/object_renderer.rb b/lib/banzai/object_renderer.rb index 9aef807c152..9f8eb0931b8 100644 --- a/lib/banzai/object_renderer.rb +++ b/lib/banzai/object_renderer.rb @@ -1,28 +1,32 @@ module Banzai - # Class for rendering multiple objects (e.g. Note instances) in a single pass. + # Class for rendering multiple objects (e.g. Note instances) in a single pass, + # using +render_field+ to benefit from caching in the database. Rendering and + # redaction are both performed. # - # 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`. + # The unredacted HTML is generated according to the usual +render_field+ + # policy, so specify the pipeline and any other context options on the model. + # + # The *redacted* (i.e., suitable for use) HTML is placed in an attribute + # named "redacted_<foo>", where <foo> is the name of the cache field for the + # chosen attribute. + # + # 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 - # 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. + # project - A Project to use for 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 = {}) + # context - A Hash containing extra attributes to use during redaction + def initialize(project, user = nil, redaction_context = {}) @project = project @user = user - @raw_context = raw_context + @redaction_context = redaction_context end # Renders and redacts an Array of objects. # - # objects - The objects to render + # objects - The objects to render. # attribute - The attribute containing the raw Markdown to render. # # Returns the same input objects. @@ -32,7 +36,7 @@ module Banzai objects.each_with_index do |object, index| redacted_data = redacted[index] - object.__send__("#{attribute}_html=", redacted_data[:document].to_html.html_safe) + object.__send__("redacted_#{attribute}_html=", redacted_data[:document].to_html.html_safe) object.user_visible_reference_count = redacted_data[:visible_reference_count] end end @@ -53,12 +57,8 @@ module Banzai # 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 = base_context.dup + context = context.merge(object.banzai_render_context(attribute)) context end @@ -66,21 +66,16 @@ module Banzai # # Returns an Array of `Nokogiri::HTML::Document`. def render_attributes(objects, attribute) - strings_and_contexts = objects.map do |object| + objects.map do |object| + string = Banzai.render_field(object, attribute) context = context_for(object, attribute) - string = object.__send__(attribute) - - { text: string, context: context } - end - - Banzai.cache_collection_render(strings_and_contexts).each_with_index.map do |html, index| - Banzai::Pipeline[:relative_link].to_document(html, strings_and_contexts[index][:context]) + Banzai::Pipeline[:relative_link].to_document(string, context) end end def base_context - @base_context ||= @raw_context.merge(current_user: user, project: project) + @base_context ||= @redaction_context.merge(current_user: user, project: project) end end end diff --git a/lib/banzai/pipeline/single_line_pipeline.rb b/lib/banzai/pipeline/single_line_pipeline.rb index ba2555df98d..30bc035d085 100644 --- a/lib/banzai/pipeline/single_line_pipeline.rb +++ b/lib/banzai/pipeline/single_line_pipeline.rb @@ -3,6 +3,7 @@ module Banzai class SingleLinePipeline < GfmPipeline def self.filters @filters ||= FilterArray[ + Filter::HTMLEntityFilter, Filter::SanitizationFilter, Filter::EmojiFilter, diff --git a/lib/banzai/renderer.rb b/lib/banzai/renderer.rb index a4ae27eefd8..6924a293da8 100644 --- a/lib/banzai/renderer.rb +++ b/lib/banzai/renderer.rb @@ -31,6 +31,34 @@ module Banzai end end + # 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 learned from the passed-in object by calling + # #banzai_render_context(field), and cannot be changed. Use #render, passing + # it the field text, if a custom rendering is needed. The generated context + # is returned along with the HTML. + def render_field(object, field) + html_field = object.markdown_cache_field_for(field) + + html = object.__send__(html_field) + return html if html.present? + + html = cacheless_render_field(object, field) + object.update_column(html_field, html) unless object.new_record? || object.destroyed? + + html + end + + # Same as +render_field+, but without consulting or updating the cache field + def cacheless_render_field(object, field) + text = object.__send__(field) + context = object.banzai_render_context(field) + + cacheless_render(text, context) + end + # Perform multiple render from an Array of Markdown String into an # Array of HTML-safe String of HTML. # diff --git a/lib/tasks/cache.rake b/lib/tasks/cache.rake index 2214f855200..a95a3455a4a 100644 --- a/lib/tasks/cache.rake +++ b/lib/tasks/cache.rake @@ -1,22 +1,33 @@ namespace :cache do - CLEAR_BATCH_SIZE = 1000 # There seems to be no speedup when pushing beyond 1,000 - REDIS_SCAN_START_STOP = '0' # Magic value, see http://redis.io/commands/scan + namespace :clear do + REDIS_CLEAR_BATCH_SIZE = 1000 # There seems to be no speedup when pushing beyond 1,000 + REDIS_SCAN_START_STOP = '0' # Magic value, see http://redis.io/commands/scan - desc "GitLab | Clear redis cache" - task :clear => :environment do - Gitlab::Redis.with do |redis| - cursor = REDIS_SCAN_START_STOP - loop do - cursor, keys = redis.scan( - cursor, - match: "#{Gitlab::Redis::CACHE_NAMESPACE}*", - count: CLEAR_BATCH_SIZE - ) - - redis.del(*keys) if keys.any? - - break if cursor == REDIS_SCAN_START_STOP + desc "GitLab | Clear redis cache" + task redis: :environment do + Gitlab::Redis.with do |redis| + cursor = REDIS_SCAN_START_STOP + loop do + cursor, keys = redis.scan( + cursor, + match: "#{Gitlab::Redis::CACHE_NAMESPACE}*", + count: REDIS_CLEAR_BATCH_SIZE + ) + + redis.del(*keys) if keys.any? + + break if cursor == REDIS_SCAN_START_STOP + end end end + + desc "GitLab | Clear database cache (in the background)" + task db: :environment do + ClearDatabaseCacheWorker.perform_async + end + + task all: [:db, :redis] end + + task clear: 'cache:clear:all' end diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index 873d3fcb5af..331172445e4 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -9,6 +9,9 @@ FactoryGirl.define do namespace creator + # Behaves differently to nil due to cache_has_external_issue_tracker + has_external_issue_tracker false + trait :public do visibility_level Gitlab::VisibilityLevel::PUBLIC end @@ -92,6 +95,8 @@ FactoryGirl.define do end factory :redmine_project, parent: :project do + has_external_issue_tracker true + after :create do |project| project.create_redmine_service( active: true, @@ -105,6 +110,8 @@ FactoryGirl.define do end factory :jira_project, parent: :project do + has_external_issue_tracker true + after :create do |project| project.create_jira_service( active: true, diff --git a/spec/helpers/broadcast_messages_helper_spec.rb b/spec/helpers/broadcast_messages_helper_spec.rb index 157cc4665a2..c6e3c5c2368 100644 --- a/spec/helpers/broadcast_messages_helper_spec.rb +++ b/spec/helpers/broadcast_messages_helper_spec.rb @@ -7,7 +7,7 @@ describe BroadcastMessagesHelper do end it 'includes the current message' do - current = double(message: 'Current Message') + current = BroadcastMessage.new(message: 'Current Message') allow(helper).to receive(:broadcast_message_style).and_return(nil) @@ -15,7 +15,7 @@ describe BroadcastMessagesHelper do end it 'includes custom style' do - current = double(message: 'Current Message') + current = BroadcastMessage.new(message: 'Current Message') allow(helper).to receive(:broadcast_message_style).and_return('foo') diff --git a/spec/lib/banzai/filter/html_entity_filter_spec.rb b/spec/lib/banzai/filter/html_entity_filter_spec.rb new file mode 100644 index 00000000000..6dc4a970071 --- /dev/null +++ b/spec/lib/banzai/filter/html_entity_filter_spec.rb @@ -0,0 +1,14 @@ +require 'spec_helper' + +describe Banzai::Filter::HTMLEntityFilter, lib: true do + include FilterSpecHelper + + let(:unescaped) { 'foo <strike attr="foo">&&&</strike>' } + let(:escaped) { 'foo <strike attr="foo">&&&</strike>' } + + it 'converts common entities to their HTML-escaped equivalents' do + output = filter(unescaped) + + expect(output).to eq(escaped) + end +end diff --git a/spec/lib/banzai/note_renderer_spec.rb b/spec/lib/banzai/note_renderer_spec.rb index 98f76f36fd5..49556074278 100644 --- a/spec/lib/banzai/note_renderer_spec.rb +++ b/spec/lib/banzai/note_renderer_spec.rb @@ -12,8 +12,7 @@ describe Banzai::NoteRenderer do with(project, user, requested_path: 'foo', project_wiki: wiki, - ref: 'bar', - pipeline: :note). + ref: 'bar'). and_call_original expect_any_instance_of(Banzai::ObjectRenderer). diff --git a/spec/lib/banzai/object_renderer_spec.rb b/spec/lib/banzai/object_renderer_spec.rb index bcdb95250ca..f5ff236105e 100644 --- a/spec/lib/banzai/object_renderer_spec.rb +++ b/spec/lib/banzai/object_renderer_spec.rb @@ -4,10 +4,18 @@ describe Banzai::ObjectRenderer do let(:project) { create(:empty_project) } let(:user) { project.owner } + def fake_object(attrs = {}) + object = double(attrs.merge("new_record?": true, "destroyed?": true)) + allow(object).to receive(:markdown_cache_field_for).with(:note).and_return(:note_html) + allow(object).to receive(:banzai_render_context).with(:note).and_return(project: nil, author: nil) + allow(object).to receive(:update_column).with(:note_html, anything).and_return(true) + object + end + 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) + object = fake_object(note: 'hello', note_html: nil) expect(renderer).to receive(:render_objects).with([object], :note). and_call_original @@ -16,7 +24,7 @@ describe Banzai::ObjectRenderer do with(an_instance_of(Array)). and_call_original - expect(object).to receive(:note_html=).with('<p>hello</p>') + expect(object).to receive(:redacted_note_html=).with('<p>hello</p>') expect(object).to receive(:user_visible_reference_count=).with(0) renderer.render([object], :note) @@ -25,7 +33,7 @@ describe Banzai::ObjectRenderer do describe '#render_objects' do it 'renders an Array of objects' do - object = double(:object, note: 'hello') + object = fake_object(note: 'hello', note_html: nil) renderer = described_class.new(project, user) @@ -57,49 +65,29 @@ describe Banzai::ObjectRenderer do end describe '#context_for' do - let(:object) { double(:object, note: 'hello') } + let(:object) { fake_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 + it 'includes the banzai render context for the object' do + expect(object).to receive(:banzai_render_context).with(:note).and_return(foo: :bar) 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 + expect(context).to have_key(:foo) + expect(context[:foo]).to eq(:bar) end end describe '#render_attributes' do it 'renders the attribute of a list of objects' do - objects = [double(:doc, note: 'hello'), double(:doc, note: 'bye')] - renderer = described_class.new(project, user, pipeline: :note) + objects = [fake_object(note: 'hello', note_html: nil), fake_object(note: 'bye', note_html: nil)] + renderer = described_class.new(project, user) - expect(Banzai).to receive(:cache_collection_render). - with([ - { text: 'hello', context: renderer.context_for(objects[0], :note) }, - { text: 'bye', context: renderer.context_for(objects[1], :note) } - ]). - and_call_original + objects.each do |object| + expect(Banzai).to receive(:render_field).with(object, :note).and_call_original + end docs = renderer.render_attributes(objects, :note) @@ -114,17 +102,13 @@ describe Banzai::ObjectRenderer do objects = [] renderer = described_class.new(project, user, pipeline: :note) - expect(Banzai).to receive(:cache_collection_render). - with([]). - and_call_original - expect(renderer.render_attributes(objects, :note)).to eq([]) end end describe '#base_context' do let(:context) do - described_class.new(project, user, pipeline: :note).base_context + described_class.new(project, user, foo: :bar).base_context end it 'returns a Hash' do @@ -132,7 +116,7 @@ describe Banzai::ObjectRenderer do end it 'includes the custom attributes' do - expect(context[:pipeline]).to eq(:note) + expect(context[:foo]).to eq(:bar) end it 'includes the current user' do diff --git a/spec/lib/banzai/renderer_spec.rb b/spec/lib/banzai/renderer_spec.rb new file mode 100644 index 00000000000..aaa6b12e67e --- /dev/null +++ b/spec/lib/banzai/renderer_spec.rb @@ -0,0 +1,74 @@ +require 'spec_helper' + +describe Banzai::Renderer do + def expect_render(project = :project) + expected_context = { project: project } + expect(renderer).to receive(:cacheless_render) { :html }.with(:markdown, expected_context) + end + + def expect_cache_update + expect(object).to receive(:update_column).with("field_html", :html) + end + + def fake_object(*features) + markdown = :markdown if features.include?(:markdown) + html = :html if features.include?(:html) + + object = double( + "object", + banzai_render_context: { project: :project }, + field: markdown, + field_html: html + ) + + allow(object).to receive(:markdown_cache_field_for).with(:field).and_return("field_html") + allow(object).to receive(:new_record?).and_return(features.include?(:new)) + allow(object).to receive(:destroyed?).and_return(features.include?(:destroyed)) + + object + end + + describe "#render_field" do + let(:renderer) { Banzai::Renderer } + let(:subject) { renderer.render_field(object, :field) } + + context "with an empty cache" do + let(:object) { fake_object(:markdown) } + it "caches and returns the result" do + expect_render + expect_cache_update + expect(subject).to eq(:html) + end + end + + context "with a filled cache" do + let(:object) { fake_object(:markdown, :html) } + + it "uses the cache" do + expect_render.never + expect_cache_update.never + should eq(:html) + end + end + + context "new object" do + let(:object) { fake_object(:new, :markdown) } + + it "doesn't cache the result" do + expect_render + expect_cache_update.never + expect(subject).to eq(:html) + end + end + + context "destroyed object" do + let(:object) { fake_object(:destroyed, :markdown) } + + it "doesn't cache the result" do + expect_render + expect_cache_update.never + expect(subject).to eq(:html) + end + end + end +end diff --git a/spec/lib/gitlab/import_export/attribute_configuration_spec.rb b/spec/lib/gitlab/import_export/attribute_configuration_spec.rb index 2e19d590d83..ea65a5dfed1 100644 --- a/spec/lib/gitlab/import_export/attribute_configuration_spec.rb +++ b/spec/lib/gitlab/import_export/attribute_configuration_spec.rb @@ -26,10 +26,11 @@ describe 'Import/Export attribute configuration', lib: true do it 'has no new columns' do relation_names.each do |relation_name| relation_class = relation_class_for_name(relation_name) + relation_attributes = relation_class.new.attributes.keys expect(safe_model_attributes[relation_class.to_s]).not_to be_nil, "Expected exported class #{relation_class} to exist in safe_model_attributes" - current_attributes = parsed_attributes(relation_name, relation_class.attribute_names) + current_attributes = parsed_attributes(relation_name, relation_attributes) safe_attributes = safe_model_attributes[relation_class.to_s] new_attributes = current_attributes - safe_attributes diff --git a/spec/models/abuse_report_spec.rb b/spec/models/abuse_report_spec.rb index 305f8bc88cc..c4486a32082 100644 --- a/spec/models/abuse_report_spec.rb +++ b/spec/models/abuse_report_spec.rb @@ -9,6 +9,10 @@ RSpec.describe AbuseReport, type: :model do describe 'associations' do it { is_expected.to belong_to(:reporter).class_name('User') } it { is_expected.to belong_to(:user) } + + it "aliases reporter to author" do + expect(subject.author).to be(subject.reporter) + end end describe 'validations' do diff --git a/spec/models/concerns/cache_markdown_field_spec.rb b/spec/models/concerns/cache_markdown_field_spec.rb new file mode 100644 index 00000000000..15cd3a7ed70 --- /dev/null +++ b/spec/models/concerns/cache_markdown_field_spec.rb @@ -0,0 +1,181 @@ +require 'spec_helper' + +describe CacheMarkdownField do + CacheMarkdownField::CACHING_CLASSES << "ThingWithMarkdownFields" + + # The minimum necessary ActiveModel to test this concern + class ThingWithMarkdownFields + include ActiveModel::Model + include ActiveModel::Dirty + + include ActiveModel::Serialization + + class_attribute :attribute_names + self.attribute_names = [] + + def attributes + attribute_names.each_with_object({}) do |name, hsh| + hsh[name.to_s] = send(name) + end + end + + extend ActiveModel::Callbacks + define_model_callbacks :save + + include CacheMarkdownField + cache_markdown_field :foo + cache_markdown_field :baz, pipeline: :single_line + + def self.add_attr(attr_name) + self.attribute_names += [attr_name] + define_attribute_methods(attr_name) + attr_reader(attr_name) + define_method("#{attr_name}=") do |val| + send("#{attr_name}_will_change!") unless val == send(attr_name) + instance_variable_set("@#{attr_name}", val) + end + end + + [:foo, :foo_html, :bar, :baz, :baz_html].each do |attr_name| + add_attr(attr_name) + end + + def initialize(*) + super + + # Pretend new is load + clear_changes_information + end + + def save + run_callbacks :save do + changes_applied + end + end + end + + CacheMarkdownField::CACHING_CLASSES.delete("ThingWithMarkdownFields") + + def thing_subclass(new_attr) + Class.new(ThingWithMarkdownFields) { add_attr(new_attr) } + end + + let(:markdown) { "`Foo`" } + let(:html) { "<p><code>Foo</code></p>" } + + let(:updated_markdown) { "`Bar`" } + let(:updated_html) { "<p><code>Bar</code></p>" } + + subject { ThingWithMarkdownFields.new(foo: markdown, foo_html: html) } + + describe ".attributes" do + it "excludes cache attributes" do + expect(thing_subclass(:qux).new.attributes.keys.sort).to eq(%w[bar baz foo qux]) + end + end + + describe ".cache_markdown_field" do + it "refuses to allow untracked classes" do + expect { thing_subclass(:qux).__send__(:cache_markdown_field, :qux) }.to raise_error(RuntimeError) + end + end + + context "an unchanged markdown field" do + before do + subject.foo = subject.foo + subject.save + end + + it { expect(subject.foo).to eq(markdown) } + it { expect(subject.foo_html).to eq(html) } + it { expect(subject.foo_html_changed?).not_to be_truthy } + end + + context "a changed markdown field" do + before do + subject.foo = updated_markdown + subject.save + end + + it { expect(subject.foo_html).to eq(updated_html) } + end + + context "a non-markdown field changed" do + before do + subject.bar = "OK" + subject.save + end + + it { expect(subject.bar).to eq("OK") } + it { expect(subject.foo).to eq(markdown) } + it { expect(subject.foo_html).to eq(html) } + end + + describe '#banzai_render_context' do + it "sets project to nil if the object lacks a project" do + context = subject.banzai_render_context(:foo) + expect(context).to have_key(:project) + expect(context[:project]).to be_nil + end + + it "excludes author if the object lacks an author" do + context = subject.banzai_render_context(:foo) + expect(context).not_to have_key(:author) + end + + it "raises if the context for an unrecognised field is requested" do + expect{subject.banzai_render_context(:not_found)}.to raise_error(ArgumentError) + end + + it "includes the pipeline" do + context = subject.banzai_render_context(:baz) + expect(context[:pipeline]).to eq(:single_line) + end + + it "returns copies of the context template" do + template = subject.cached_markdown_fields[:baz] + copy = subject.banzai_render_context(:baz) + expect(copy).not_to be(template) + end + + context "with a project" do + subject { thing_subclass(:project).new(foo: markdown, foo_html: html, project: :project) } + + it "sets the project in the context" do + context = subject.banzai_render_context(:foo) + expect(context).to have_key(:project) + expect(context[:project]).to eq(:project) + end + + it "invalidates the cache when project changes" do + subject.project = :new_project + allow(Banzai::Renderer).to receive(:cacheless_render_field).and_return(updated_html) + + subject.save + + expect(subject.foo_html).to eq(updated_html) + expect(subject.baz_html).to eq(updated_html) + end + end + + context "with an author" do + subject { thing_subclass(:author).new(foo: markdown, foo_html: html, author: :author) } + + it "sets the author in the context" do + context = subject.banzai_render_context(:foo) + expect(context).to have_key(:author) + expect(context[:author]).to eq(:author) + end + + it "invalidates the cache when author changes" do + subject.author = :new_author + allow(Banzai::Renderer).to receive(:cacheless_render_field).and_return(updated_html) + + subject.save + + expect(subject.foo_html).to eq(updated_html) + expect(subject.baz_html).to eq(updated_html) + end + end + end +end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index e52d4aaf884..381d14ed21a 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -518,7 +518,7 @@ describe Project, models: true do end describe '#cache_has_external_issue_tracker' do - let(:project) { create(:project) } + let(:project) { create(:project, has_external_issue_tracker: nil) } it 'stores true if there is any external_issue_tracker' do services = double(:service, external_issue_trackers: [RedmineService.new]) diff --git a/spec/models/service_spec.rb b/spec/models/service_spec.rb index ed1bc9271ae..43937a54b2c 100644 --- a/spec/models/service_spec.rb +++ b/spec/models/service_spec.rb @@ -238,7 +238,7 @@ describe Service, models: true do it "updates the has_external_issue_tracker boolean" do expect do service.save! - end.to change { service.project.has_external_issue_tracker }.from(nil).to(true) + end.to change { service.project.has_external_issue_tracker }.from(false).to(true) end end diff --git a/spec/models/snippet_spec.rb b/spec/models/snippet_spec.rb index e6bc5296398..f62f6bacbaa 100644 --- a/spec/models/snippet_spec.rb +++ b/spec/models/snippet_spec.rb @@ -46,6 +46,13 @@ describe Snippet, models: true do end end + describe "#content_html_invalidated?" do + let(:snippet) { create(:snippet, content: "md", content_html: "html", file_name: "foo.md") } + it "invalidates the HTML cache of content when the filename changes" do + expect { snippet.file_name = "foo.rb" }.to change { snippet.content_html_invalidated? }.from(false).to(true) + end + end + describe '.search' do let(:snippet) { create(:snippet) } diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 4a0d727faea..861eb12b94c 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -232,7 +232,7 @@ describe API::API, api: true do post api('/projects', user), project project.each_pair do |k, v| - next if %i{ issues_enabled merge_requests_enabled wiki_enabled }.include?(k) + next if %i[has_external_issue_tracker issues_enabled merge_requests_enabled wiki_enabled].include?(k) expect(json_response[k.to_s]).to eq(v) end @@ -360,7 +360,7 @@ describe API::API, api: true do post api("/projects/user/#{user.id}", admin), project project.each_pair do |k, v| - next if k == :path + next if %i[has_external_issue_tracker path].include?(k) expect(json_response[k.to_s]).to eq(v) end end diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index 22991c5bc86..8e3e12114f2 100644 --- a/spec/services/git_push_service_spec.rb +++ b/spec/services/git_push_service_spec.rb @@ -448,6 +448,8 @@ describe GitPushService, services: true do let(:jira_tracker) { project.create_jira_service if project.jira_service.nil? } before do + # project.create_jira_service doesn't seem to invalidate the cache here + project.has_external_issue_tracker = true jira_service_settings WebMock.stub_request(:post, jira_api_transition_url) diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index e49a0d5e553..ee53e110aee 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -60,7 +60,10 @@ describe MergeRequests::MergeService, services: true do let(:jira_tracker) { project.create_jira_service } - before { jira_service_settings } + before do + project.update_attributes!(has_external_issue_tracker: true) + jira_service_settings + end it 'closes issues on JIRA issue tracker' do jira_issue = ExternalIssue.new('JIRA-123', project) diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index d1a47ea9b6f..304d4e62396 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -531,12 +531,12 @@ describe SystemNoteService, services: true do include JiraServiceHelper describe 'JIRA integration' do - let(:project) { create(:project) } + let(:project) { create(:jira_project) } let(:author) { create(:user) } let(:issue) { create(:issue, project: project) } let(:mergereq) { create(:merge_request, :simple, target_project: project, source_project: project) } let(:jira_issue) { ExternalIssue.new("JIRA-1", project)} - let(:jira_tracker) { project.create_jira_service if project.jira_service.nil? } + let(:jira_tracker) { project.jira_service } let(:commit) { project.commit } context 'in JIRA issue tracker' do @@ -545,10 +545,6 @@ describe SystemNoteService, services: true do WebMock.stub_request(:post, jira_api_comment_url) end - after do - jira_tracker.destroy! - end - describe "new reference" do before do WebMock.stub_request(:get, jira_api_comment_url).to_return(body: jira_issue_comments) @@ -578,10 +574,6 @@ describe SystemNoteService, services: true do WebMock.stub_request(:get, jira_api_comment_url).to_return(body: jira_issue_comments) end - after do - jira_tracker.destroy! - end - subject { described_class.cross_reference(jira_issue, issue, author) } it { is_expected.to eq(jira_status_message) } |