From e94cd6fdfe43d9128d37a539cf84f4388c5cf970 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Thu, 6 Oct 2016 22:17:11 +0100
Subject: [PATCH] Add markdown cache columns to the database, but don't use
them yet
This commit adds a number of _html columns and, with the exception of Note,
starts updating them whenever the content of their partner fields changes.
Note has a collision with the note_html attr_accessor; that will be fixed later
A background worker for clearing these cache columns is also introduced - use
`rake cache:clear` to set it off. You can clear the database or Redis caches
separately by running `rake cache:clear:db` or `rake cache:clear:redis`,
respectively.
---
app/helpers/gitlab_markdown_helper.rb | 30 ++-
app/models/abuse_report.rb | 7 +
app/models/appearance.rb | 4 +
app/models/application_setting.rb | 7 +
app/models/broadcast_message.rb | 3 +
app/models/concerns/cache_markdown_field.rb | 131 +++++++++++++
app/models/concerns/issuable.rb | 4 +
app/models/global_label.rb | 4 +
app/models/global_milestone.rb | 5 +
app/models/label.rb | 3 +
app/models/milestone.rb | 4 +
app/models/namespace.rb | 3 +
app/models/project.rb | 3 +
app/models/release.rb | 4 +
app/models/snippet.rb | 10 +
app/workers/clear_database_cache_worker.rb | 23 +++
config/initializers/ar5_batching.rb | 41 ++++
...160829114652_add_markdown_cache_columns.rb | 38 ++++
db/schema.rb | 36 +++-
lib/banzai.rb | 4 +
lib/banzai/renderer.rb | 28 +++
lib/tasks/cache.rake | 43 +++--
spec/factories/projects.rb | 7 +
spec/lib/banzai/renderer_spec.rb | 74 +++++++
.../attribute_configuration_spec.rb | 3 +-
spec/models/abuse_report_spec.rb | 4 +
.../concerns/cache_markdown_field_spec.rb | 181 ++++++++++++++++++
spec/models/project_spec.rb | 2 +-
spec/models/service_spec.rb | 2 +-
spec/models/snippet_spec.rb | 7 +
spec/requests/api/projects_spec.rb | 4 +-
spec/services/git_push_service_spec.rb | 2 +
.../merge_requests/merge_service_spec.rb | 5 +-
spec/services/system_note_service_spec.rb | 12 +-
34 files changed, 689 insertions(+), 49 deletions(-)
create mode 100644 app/models/concerns/cache_markdown_field.rb
create mode 100644 app/workers/clear_database_cache_worker.rb
create mode 100644 config/initializers/ar5_batching.rb
create mode 100644 db/migrate/20160829114652_add_markdown_cache_columns.rb
create mode 100644 spec/lib/banzai/renderer_spec.rb
create mode 100644 spec/models/concerns/cache_markdown_field_spec.rb
diff --git a/app/helpers/gitlab_markdown_helper.rb b/app/helpers/gitlab_markdown_helper.rb
index 1a259656f31..d24680b8617 100644
--- a/app/helpers/gitlab_markdown_helper.rb
+++ b/app/helpers/gitlab_markdown_helper.rb
@@ -51,17 +51,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)),
+ def markdown_field(object, field)
+ object = object.for_display if object.respond_to?(:for_display)
+ return "" unless object.present?
- # RelativeLinkFilter
- requested_path: @path,
- project_wiki: @project_wiki,
- ref: @ref
- )
-
- 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 +194,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/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/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/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/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/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) }
--
GitLab