From 2eecfd8f9d111c6518930b818a16daea8263b37f Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Tue, 4 Jun 2019 20:59:48 -0800 Subject: Use Redis for CacheMarkDownField on non AR models This allows using `CacheMarkdownField` for models that are not backed by ActiveRecord. When the including class inherits `ActiveRecord::Base` we include `Gitlab::MarkdownCache::ActiveRecord::Extension`. This will cause the markdown fields to be rendered and the generated HTML stored in a `_html` attribute on the record. We also store the version used for generating the markdown. All other classes that include this model will include the `Gitlab::MarkdownCache::Redis::Extension`. This add the `_html` attributes to that model and will generate the html in them. The generated HTML will be cached in redis under the key `markdown_cache::`. The class this included in must therefore respond to `id`. --- app/models/commit.rb | 11 +- app/models/concerns/cache_markdown_field.rb | 88 +--- .../54140-non-ar-cache-commit-markdown.yml | 5 + lib/banzai/commit_renderer.rb | 2 +- lib/gitlab/markdown_cache.rb | 12 + .../markdown_cache/active_record/extension.rb | 55 +++ lib/gitlab/markdown_cache/field_data.rb | 35 ++ lib/gitlab/markdown_cache/redis/extension.rb | 63 +++ lib/gitlab/markdown_cache/redis/store.rb | 56 +++ .../markdown/gitlab_flavored_markdown_spec.rb | 3 +- spec/helpers/markup_helper_spec.rb | 3 +- spec/lib/banzai/commit_renderer_spec.rb | 4 +- spec/lib/banzai/object_renderer_spec.rb | 28 +- spec/lib/banzai/renderer_spec.rb | 14 +- .../markdown_cache/active_record/extension_spec.rb | 177 ++++++++ spec/lib/gitlab/markdown_cache/field_data_spec.rb | 15 + .../gitlab/markdown_cache/redis/extension_spec.rb | 76 ++++ spec/lib/gitlab/markdown_cache/redis/store_spec.rb | 68 ++++ spec/models/concerns/cache_markdown_field_spec.rb | 446 +++++++-------------- spec/models/resource_label_event_spec.rb | 4 +- 20 files changed, 760 insertions(+), 405 deletions(-) create mode 100644 changelogs/unreleased/54140-non-ar-cache-commit-markdown.yml create mode 100644 lib/gitlab/markdown_cache.rb create mode 100644 lib/gitlab/markdown_cache/active_record/extension.rb create mode 100644 lib/gitlab/markdown_cache/field_data.rb create mode 100644 lib/gitlab/markdown_cache/redis/extension.rb create mode 100644 lib/gitlab/markdown_cache/redis/store.rb create mode 100644 spec/lib/gitlab/markdown_cache/active_record/extension_spec.rb create mode 100644 spec/lib/gitlab/markdown_cache/field_data_spec.rb create mode 100644 spec/lib/gitlab/markdown_cache/redis/extension_spec.rb create mode 100644 spec/lib/gitlab/markdown_cache/redis/store_spec.rb diff --git a/app/models/commit.rb b/app/models/commit.rb index f412d252e5c..fa0bf36ba49 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -13,6 +13,7 @@ class Commit include StaticModel include Presentable include ::Gitlab::Utils::StrongMemoize + include CacheMarkdownField attr_mentionable :safe_message, pipeline: :single_line @@ -37,13 +38,9 @@ class Commit # Used by GFM to match and present link extensions on node texts and hrefs. LINK_EXTENSION_PATTERN = /(patch)/.freeze - def banzai_render_context(field) - pipeline = field == :description ? :commit_description : :single_line - context = { pipeline: pipeline, project: self.project } - context[:author] = self.author if self.author - - context - end + cache_markdown_field :title, pipeline: :single_line + cache_markdown_field :full_title, pipeline: :single_line + cache_markdown_field :description, pipeline: :commit_description class << self def decorate(commits, project) diff --git a/app/models/concerns/cache_markdown_field.rb b/app/models/concerns/cache_markdown_field.rb index f90cd1ea690..42203a5f214 100644 --- a/app/models/concerns/cache_markdown_field.rb +++ b/app/models/concerns/cache_markdown_field.rb @@ -13,43 +13,9 @@ module CacheMarkdownField extend ActiveSupport::Concern - # Increment this number every time the renderer changes its output - CACHE_COMMONMARK_VERSION_START = 10 - CACHE_COMMONMARK_VERSION = 16 - # changes to these attributes cause the cache to be invalidates INVALIDATED_BY = %w[author project].freeze - # Knows about the relationship between markdown and html field names, and - # stores the rendering contexts for the latter - class FieldData - def initialize - @data = {} - end - - delegate :[], :[]=, to: :@data - - def markdown_fields - @data.keys - end - - def html_field(markdown_field) - "#{markdown_field}_html" - end - - def html_fields - markdown_fields.map { |field| html_field(field) } - end - - def html_fields_whitelisted - markdown_fields.each_with_object([]) do |field, fields| - if @data[field].fetch(:whitelisted, false) - fields << html_field(field) - end - end - end - end - def skip_project_check? false end @@ -85,24 +51,22 @@ module CacheMarkdownField end.to_h updates['cached_markdown_version'] = latest_cached_markdown_version - updates.each {|html_field, data| write_attribute(html_field, data) } + updates.each { |field, data| write_markdown_field(field, data) } end def refresh_markdown_cache! updates = refresh_markdown_cache - return unless persisted? && Gitlab::Database.read_write? - - update_columns(updates) + save_markdown(updates) end def cached_html_up_to_date?(markdown_field) - html_field = cached_markdown_fields.html_field(markdown_field) + return false if cached_html_for(markdown_field).nil? && __send__(markdown_field).present? # rubocop:disable GitlabSecurity/PublicSend - return false if cached_html_for(markdown_field).nil? && !__send__(markdown_field).nil? # rubocop:disable GitlabSecurity/PublicSend + html_field = cached_markdown_fields.html_field(markdown_field) - markdown_changed = attribute_changed?(markdown_field) || false - html_changed = attribute_changed?(html_field) || false + markdown_changed = markdown_field_changed?(markdown_field) + html_changed = markdown_field_changed?(html_field) latest_cached_markdown_version == cached_markdown_version && (html_changed || markdown_changed == html_changed) @@ -117,21 +81,21 @@ module CacheMarkdownField end def cached_html_for(markdown_field) - raise ArgumentError.new("Unknown field: #{field}") unless + raise ArgumentError.new("Unknown field: #{markdown_field}") unless cached_markdown_fields.markdown_fields.include?(markdown_field) __send__(cached_markdown_fields.html_field(markdown_field)) # rubocop:disable GitlabSecurity/PublicSend end def latest_cached_markdown_version - @latest_cached_markdown_version ||= (CacheMarkdownField::CACHE_COMMONMARK_VERSION << 16) | local_version + @latest_cached_markdown_version ||= (Gitlab::MarkdownCache::CACHE_COMMONMARK_VERSION << 16) | local_version end def local_version # because local_markdown_version is stored in application_settings which # uses cached_markdown_version too, we check explicitly to avoid # endless loop - return local_markdown_version if has_attribute?(:local_markdown_version) + return local_markdown_version if respond_to?(:has_attribute?) && has_attribute?(:local_markdown_version) settings = Gitlab::CurrentSettings.current_application_settings @@ -150,32 +114,14 @@ module CacheMarkdownField included do cattr_reader :cached_markdown_fields do - FieldData.new + Gitlab::MarkdownCache::FieldData.new 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 - html_fields = cached_markdown_fields.html_fields - whitelisted = cached_markdown_fields.html_fields_whitelisted - exclude_fields = html_fields - whitelisted - - exclude_fields.each do |field| - attrs.delete(field) - end - - if whitelisted.empty? - attrs.delete('cached_markdown_version') - end - - attrs + if self < ActiveRecord::Base + include Gitlab::MarkdownCache::ActiveRecord::Extension + else + prepend Gitlab::MarkdownCache::Redis::Extension end - - # Using before_update here conflicts with elasticsearch-model somehow - before_create :refresh_markdown_cache, if: :invalidated_markdown_cache? - before_update :refresh_markdown_cache, if: :invalidated_markdown_cache? end class_methods do @@ -193,10 +139,8 @@ module CacheMarkdownField # 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, *INVALIDATED_BY] - invalidations.delete(markdown_field.to_s) if changed_fields.include?("#{markdown_field}_html") - + invalidations = changed_markdown_fields & [markdown_field.to_s, *INVALIDATED_BY] + invalidations.delete(markdown_field.to_s) if changed_markdown_fields.include?("#{markdown_field}_html") !invalidations.empty? || !cached_html_up_to_date?(markdown_field) end end diff --git a/changelogs/unreleased/54140-non-ar-cache-commit-markdown.yml b/changelogs/unreleased/54140-non-ar-cache-commit-markdown.yml new file mode 100644 index 00000000000..efda07380a4 --- /dev/null +++ b/changelogs/unreleased/54140-non-ar-cache-commit-markdown.yml @@ -0,0 +1,5 @@ +--- +title: Use Redis for CacheMarkDownField on non AR models +merge_request: 29054 +author: +type: performance diff --git a/lib/banzai/commit_renderer.rb b/lib/banzai/commit_renderer.rb index f346151a3c1..2acc9d13f07 100644 --- a/lib/banzai/commit_renderer.rb +++ b/lib/banzai/commit_renderer.rb @@ -2,7 +2,7 @@ module Banzai module CommitRenderer - ATTRIBUTES = [:description, :title].freeze + ATTRIBUTES = [:description, :title, :full_title].freeze def self.render(commits, project, user = nil) obj_renderer = ObjectRenderer.new(user: user, default_project: project) diff --git a/lib/gitlab/markdown_cache.rb b/lib/gitlab/markdown_cache.rb new file mode 100644 index 00000000000..0354c710a3f --- /dev/null +++ b/lib/gitlab/markdown_cache.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +module Gitlab + module MarkdownCache + # Increment this number every time the renderer changes its output + CACHE_COMMONMARK_VERSION_START = 10 + CACHE_COMMONMARK_VERSION = 16 + + BaseError = Class.new(StandardError) + UnsupportedClassError = Class.new(BaseError) + end +end diff --git a/lib/gitlab/markdown_cache/active_record/extension.rb b/lib/gitlab/markdown_cache/active_record/extension.rb new file mode 100644 index 00000000000..bcc3432bd31 --- /dev/null +++ b/lib/gitlab/markdown_cache/active_record/extension.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +module Gitlab + module MarkdownCache + module ActiveRecord + module Extension + extend ActiveSupport::Concern + + included do + # 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 + html_fields = cached_markdown_fields.html_fields + whitelisted = cached_markdown_fields.html_fields_whitelisted + exclude_fields = html_fields - whitelisted + + exclude_fields.each do |field| + attrs.delete(field) + end + + if whitelisted.empty? + attrs.delete('cached_markdown_version') + end + + attrs + end + + # Using before_update here conflicts with elasticsearch-model somehow + before_create :refresh_markdown_cache, if: :invalidated_markdown_cache? + before_update :refresh_markdown_cache, if: :invalidated_markdown_cache? + end + + def changed_markdown_fields + changed_attributes.keys.map(&:to_s) & cached_markdown_fields.markdown_fields.map(&:to_s) + end + + def write_markdown_field(field_name, value) + write_attribute(field_name, value) + end + + def markdown_field_changed?(field_name) + attribute_changed?(field_name) + end + + def save_markdown(updates) + return unless persisted? && Gitlab::Database.read_write? + + update_columns(updates) + end + end + end + end +end diff --git a/lib/gitlab/markdown_cache/field_data.rb b/lib/gitlab/markdown_cache/field_data.rb new file mode 100644 index 00000000000..14622c0f186 --- /dev/null +++ b/lib/gitlab/markdown_cache/field_data.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +module Gitlab + module MarkdownCache + # Knows about the relationship between markdown and html field names, and + # stores the rendering contexts for the latter + class FieldData + def initialize + @data = {} + end + + delegate :[], :[]=, to: :@data + + def markdown_fields + @data.keys + end + + def html_field(markdown_field) + "#{markdown_field}_html" + end + + def html_fields + @html_fields ||= markdown_fields.map { |field| html_field(field) } + end + + def html_fields_whitelisted + markdown_fields.each_with_object([]) do |field, fields| + if @data[field].fetch(:whitelisted, false) + fields << html_field(field) + end + end + end + end + end +end diff --git a/lib/gitlab/markdown_cache/redis/extension.rb b/lib/gitlab/markdown_cache/redis/extension.rb new file mode 100644 index 00000000000..97fc23343b4 --- /dev/null +++ b/lib/gitlab/markdown_cache/redis/extension.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +module Gitlab + module MarkdownCache + module Redis + module Extension + extend ActiveSupport::Concern + + attr_reader :cached_markdown_version + + class_methods do + def cache_markdown_field(markdown_field, context = {}) + super + + # define the `[field]_html` accessor + html_field = cached_markdown_fields.html_field(markdown_field) + define_method(html_field) do + load_cached_markdown unless markdown_data_loaded? + + instance_variable_get("@#{html_field}") + end + end + end + + private + + def save_markdown(updates) + markdown_store.save(updates) + end + + def write_markdown_field(field_name, value) + instance_variable_set("@#{field_name}", value) + end + + def markdown_field_changed?(field_name) + false + end + + def changed_markdown_fields + [] + end + + def cached_markdown + @cached_data ||= markdown_store.read + end + + def load_cached_markdown + cached_markdown.each do |field_name, value| + write_markdown_field(field_name, value) + end + end + + def markdown_data_loaded? + cached_markdown_version.present? || markdown_store.loaded? + end + + def markdown_store + @store ||= Gitlab::MarkdownCache::Redis::Store.new(self) + end + end + end + end +end diff --git a/lib/gitlab/markdown_cache/redis/store.rb b/lib/gitlab/markdown_cache/redis/store.rb new file mode 100644 index 00000000000..a35eebc6a1b --- /dev/null +++ b/lib/gitlab/markdown_cache/redis/store.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +module Gitlab + module MarkdownCache + module Redis + class Store + EXPIRES_IN = 1.day + + def initialize(subject) + @subject = subject + @loaded = false + end + + def save(updates) + @loaded = false + + Gitlab::Redis::Cache.with do |r| + r.mapped_hmset(markdown_cache_key, updates) + r.expire(markdown_cache_key, EXPIRES_IN) + end + end + + def read + @loaded = true + + results = Gitlab::Redis::Cache.with do |r| + r.mapped_hmget(markdown_cache_key, *fields) + end + # The value read from redis is a string, so we're converting it back + # to an int. + results[:cached_markdown_version] = results[:cached_markdown_version].to_i + results + end + + def loaded? + @loaded + end + + private + + def fields + @fields ||= @subject.cached_markdown_fields.html_fields + [:cached_markdown_version] + end + + def markdown_cache_key + unless @subject.respond_to?(:id) + raise Gitlab::MarkdownCache::UnsupportedClassError, + "This class has no id to use for caching" + end + + "markdown_cache:#{@subject.class}:#{@subject.id}" + end + end + end + end +end diff --git a/spec/features/markdown/gitlab_flavored_markdown_spec.rb b/spec/features/markdown/gitlab_flavored_markdown_spec.rb index 6997ca48427..8fda3c7193e 100644 --- a/spec/features/markdown/gitlab_flavored_markdown_spec.rb +++ b/spec/features/markdown/gitlab_flavored_markdown_spec.rb @@ -20,8 +20,7 @@ describe "GitLab Flavored Markdown" do let(:commit) { project.commit } before do - allow_any_instance_of(Commit).to receive(:title) - .and_return("fix #{issue.to_reference}\n\nask #{fred.to_reference} for details") + create_commit("fix #{issue.to_reference}\n\nask #{fred.to_reference} for details", project, user, 'master') end it "renders title in commits#index" do diff --git a/spec/helpers/markup_helper_spec.rb b/spec/helpers/markup_helper_spec.rb index c3956ba08fd..597c8f836a9 100644 --- a/spec/helpers/markup_helper_spec.rb +++ b/spec/helpers/markup_helper_spec.rb @@ -78,7 +78,8 @@ describe MarkupHelper do let(:link) { '/commits/0a1b2c3d' } let(:issues) { create_list(:issue, 2, project: project) } - it 'handles references nested in links with all the text' do + # Clean the cache to make sure the title is re-rendered from the stubbed one + it 'handles references nested in links with all the text', :clean_gitlab_redis_cache do allow(commit).to receive(:title).and_return("This should finally fix #{issues[0].to_reference} and #{issues[1].to_reference} for real") actual = helper.link_to_markdown_field(commit, :title, link) diff --git a/spec/lib/banzai/commit_renderer_spec.rb b/spec/lib/banzai/commit_renderer_spec.rb index 1f53657c59c..316dbf052c3 100644 --- a/spec/lib/banzai/commit_renderer_spec.rb +++ b/spec/lib/banzai/commit_renderer_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Banzai::CommitRenderer do - describe '.render' do + describe '.render', :clean_gitlab_redis_cache do it 'renders a commit description and title' do user = build(:user) project = create(:project, :repository) @@ -13,7 +13,7 @@ describe Banzai::CommitRenderer do described_class::ATTRIBUTES.each do |attr| expect_any_instance_of(Banzai::ObjectRenderer).to receive(:render).with([project.commit], attr).once.and_call_original - expect(Banzai::Renderer).to receive(:cacheless_render_field).with(project.commit, attr, {}) + expect(Banzai::Renderer).to receive(:cacheless_render_field).with(project.commit, attr, { skip_project_check: false }).and_call_original end described_class.render([project.commit], project, user) diff --git a/spec/lib/banzai/object_renderer_spec.rb b/spec/lib/banzai/object_renderer_spec.rb index 3b52f6666d0..7b855251a74 100644 --- a/spec/lib/banzai/object_renderer_spec.rb +++ b/spec/lib/banzai/object_renderer_spec.rb @@ -11,7 +11,7 @@ describe Banzai::ObjectRenderer do ) end - let(:object) { Note.new(note: 'hello', note_html: '

hello

', cached_markdown_version: CacheMarkdownField::CACHE_COMMONMARK_VERSION << 16) } + let(:object) { Note.new(note: 'hello', note_html: '

hello

', cached_markdown_version: Gitlab::MarkdownCache::CACHE_COMMONMARK_VERSION << 16) } describe '#render' do context 'with cache' do @@ -60,24 +60,38 @@ describe Banzai::ObjectRenderer do end context 'without cache' do - let(:commit) { project.commit } + let(:cacheless_class) do + Class.new do + attr_accessor :title, :redacted_title_html, :project + + def banzai_render_context(field) + { project: project, pipeline: :single_line } + end + end + end + let(:cacheless_thing) do + cacheless_class.new.tap do |thing| + thing.title = "Merge branch 'branch-merged' into 'master'" + thing.project = project + end + end it 'renders and redacts an Array of objects' do - renderer.render([commit], :title) + renderer.render([cacheless_thing], :title) - expect(commit.redacted_title_html).to eq("Merge branch 'branch-merged' into 'master'") + expect(cacheless_thing.redacted_title_html).to eq("Merge branch 'branch-merged' into 'master'") end it 'calls Banzai::Redactor to perform redaction' do expect_any_instance_of(Banzai::Redactor).to receive(:redact).and_call_original - renderer.render([commit], :title) + renderer.render([cacheless_thing], :title) end it 'retrieves field content using Banzai::Renderer.cacheless_render_field' do - expect(Banzai::Renderer).to receive(:cacheless_render_field).with(commit, :title, {}).and_call_original + expect(Banzai::Renderer).to receive(:cacheless_render_field).with(cacheless_thing, :title, {}).and_call_original - renderer.render([commit], :title) + renderer.render([cacheless_thing], :title) end end end diff --git a/spec/lib/banzai/renderer_spec.rb b/spec/lib/banzai/renderer_spec.rb index 650cecfc778..aa828e2f0e9 100644 --- a/spec/lib/banzai/renderer_spec.rb +++ b/spec/lib/banzai/renderer_spec.rb @@ -11,16 +11,24 @@ describe Banzai::Renderer do object end + def fake_cacheless_object + object = double('cacheless object') + + allow(object).to receive(:respond_to?).with(:cached_markdown_fields).and_return(false) + + object + end + describe '#render_field' do let(:renderer) { described_class } context 'without cache' do - let(:commit) { create(:project, :repository).commit } + let(:commit) { fake_cacheless_object } it 'returns cacheless render field' do - expect(renderer).to receive(:cacheless_render_field).with(commit, :title, {}) + expect(renderer).to receive(:cacheless_render_field).with(commit, :field, {}) - renderer.render_field(commit, :title) + renderer.render_field(commit, :field) end end diff --git a/spec/lib/gitlab/markdown_cache/active_record/extension_spec.rb b/spec/lib/gitlab/markdown_cache/active_record/extension_spec.rb new file mode 100644 index 00000000000..6700b53e790 --- /dev/null +++ b/spec/lib/gitlab/markdown_cache/active_record/extension_spec.rb @@ -0,0 +1,177 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe Gitlab::MarkdownCache::ActiveRecord::Extension do + class ARThingWithMarkdownFields < ActiveRecord::Base + self.table_name = 'issues' + include CacheMarkdownField + cache_markdown_field :title, whitelisted: true + cache_markdown_field :description, pipeline: :single_line + + attr_accessor :author, :project + end + + let(:cache_version) { Gitlab::MarkdownCache::CACHE_COMMONMARK_VERSION << 16 } + let(:thing) { ARThingWithMarkdownFields.new(title: markdown, title_html: html, cached_markdown_version: cache_version) } + + let(:markdown) { '`Foo`' } + let(:html) { '

Foo

' } + + let(:updated_markdown) { '`Bar`' } + let(:updated_html) { '

Bar

' } + + context 'an unchanged markdown field' do + let(:thing) { ARThingWithMarkdownFields.new(title: markdown) } + + before do + thing.title = thing.title + thing.save + end + + it { expect(thing.title).to eq(markdown) } + it { expect(thing.title_html).to eq(html) } + it { expect(thing.title_html_changed?).not_to be_truthy } + it { expect(thing.cached_markdown_version).to eq(cache_version) } + end + + context 'a changed markdown field' do + let(:thing) { ARThingWithMarkdownFields.new(title: markdown, title_html: html, cached_markdown_version: cache_version) } + + before do + thing.title = updated_markdown + thing.save + end + + it { expect(thing.title_html).to eq(updated_html) } + it { expect(thing.cached_markdown_version).to eq(cache_version) } + end + + context 'when a markdown field is set repeatedly to an empty string' do + it do + expect(thing).to receive(:refresh_markdown_cache).once + thing.title = '' + thing.save + thing.title = '' + thing.save + end + end + + context 'when a markdown field is set repeatedly to a string which renders as empty html' do + it do + expect(thing).to receive(:refresh_markdown_cache).once + thing.title = '[//]: # (This is also a comment.)' + thing.save + thing.title = '[//]: # (This is also a comment.)' + thing.save + end + end + + context 'a non-markdown field changed' do + let(:thing) { ARThingWithMarkdownFields.new(title: markdown, title_html: html, cached_markdown_version: cache_version) } + + before do + thing.state = 'closed' + thing.save + end + + it { expect(thing.state).to eq('closed') } + it { expect(thing.title).to eq(markdown) } + it { expect(thing.title_html).to eq(html) } + it { expect(thing.cached_markdown_version).to eq(cache_version) } + end + + context 'version is out of date' do + let(:thing) { ARThingWithMarkdownFields.new(title: updated_markdown, title_html: html, cached_markdown_version: nil) } + + before do + thing.save + end + + it { expect(thing.title_html).to eq(updated_html) } + it { expect(thing.cached_markdown_version).to eq(cache_version) } + end + + context 'when an invalidating field is changed' do + it 'invalidates the cache when project changes' do + thing.project = :new_project + allow(Banzai::Renderer).to receive(:cacheless_render_field).and_return(updated_html) + + thing.save + + expect(thing.title_html).to eq(updated_html) + expect(thing.description_html).to eq(updated_html) + expect(thing.cached_markdown_version).to eq(cache_version) + end + + it 'invalidates the cache when author changes' do + thing.author = :new_author + allow(Banzai::Renderer).to receive(:cacheless_render_field).and_return(updated_html) + + thing.save + + expect(thing.title_html).to eq(updated_html) + expect(thing.description_html).to eq(updated_html) + expect(thing.cached_markdown_version).to eq(cache_version) + end + end + + describe '.attributes' do + it 'excludes cache attributes that is blacklisted by default' do + expect(thing.attributes.keys.sort).not_to include(%w[description_html]) + end + end + + describe '#cached_html_up_to_date?' do + let(:thing) { ARThingWithMarkdownFields.create(title: updated_markdown, title_html: html, cached_markdown_version: nil) } + subject { thing.cached_html_up_to_date?(:title) } + + it 'returns false if markdown has been changed but html has not' do + thing.title = "changed!" + + is_expected.to be_falsy + end + + it 'returns true if markdown has not been changed but html has' do + thing.title_html = updated_html + + is_expected.to be_truthy + end + + it 'returns true if markdown and html have both been changed' do + thing.title = updated_markdown + thing.title_html = updated_html + + is_expected.to be_truthy + end + + it 'returns false if the markdown field is set but the html is not' do + thing.title_html = nil + + is_expected.to be_falsy + end + end + + describe '#refresh_markdown_cache!' do + before do + thing.title = updated_markdown + end + + it 'skips saving if not persisted' do + expect(thing).to receive(:persisted?).and_return(false) + expect(thing).not_to receive(:update_columns) + + thing.refresh_markdown_cache! + end + + it 'saves the changes' do + expect(thing).to receive(:persisted?).and_return(true) + + expect(thing).to receive(:update_columns) + .with("title_html" => updated_html, + "description_html" => "", + "cached_markdown_version" => cache_version) + + thing.refresh_markdown_cache! + end + end +end diff --git a/spec/lib/gitlab/markdown_cache/field_data_spec.rb b/spec/lib/gitlab/markdown_cache/field_data_spec.rb new file mode 100644 index 00000000000..393bf85aa43 --- /dev/null +++ b/spec/lib/gitlab/markdown_cache/field_data_spec.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' + +describe Gitlab::MarkdownCache::FieldData do + subject(:field_data) { described_class.new } + + before do + field_data[:description] = { project: double('project in context') } + end + + it 'translates a markdown field name into a html field name' do + expect(field_data.html_field(:description)).to eq("description_html") + end +end diff --git a/spec/lib/gitlab/markdown_cache/redis/extension_spec.rb b/spec/lib/gitlab/markdown_cache/redis/extension_spec.rb new file mode 100644 index 00000000000..d3d3cd6f03c --- /dev/null +++ b/spec/lib/gitlab/markdown_cache/redis/extension_spec.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe Gitlab::MarkdownCache::Redis::Extension, :clean_gitlab_redis_cache do + class ThingWithMarkdownFields + include CacheMarkdownField + + def initialize(title: nil, description: nil) + @title, @description = title, description + end + + attr_reader :title, :description + + cache_markdown_field :title, pipeline: :single_line + cache_markdown_field :description + + def id + "test-markdown-cache" + end + end + + let(:cache_version) { Gitlab::MarkdownCache::CACHE_COMMONMARK_VERSION << 16 } + let(:thing) { ThingWithMarkdownFields.new(title: "`Hello`", description: "`World`") } + let(:expected_cache_key) { "markdown_cache:ThingWithMarkdownFields:test-markdown-cache" } + + it 'defines the html attributes' do + thing = ThingWithMarkdownFields.new + + expect(thing).to respond_to(:title_html, :description_html, :cached_markdown_version) + end + + it 'loads the markdown from the cache only once' do + expect(thing).to receive(:load_cached_markdown).once.and_call_original + + thing.title_html + thing.description_html + end + + it 'correctly loads the markdown if it was stored in redis' do + Gitlab::Redis::Cache.with do |r| + r.mapped_hmset(expected_cache_key, + title_html: 'hello', + description_html: 'world', + cached_markdown_version: cache_version) + end + + expect(thing.title_html).to eq('hello') + expect(thing.description_html).to eq('world') + expect(thing.cached_markdown_version).to eq(cache_version) + end + + describe "#refresh_markdown_cache!" do + it "stores the value in redis" do + expected_results = { "title_html" => "`Hello`", + "description_html" => "

World

", + "cached_markdown_version" => cache_version.to_s } + + thing.refresh_markdown_cache! + + results = Gitlab::Redis::Cache.with do |r| + r.mapped_hmget(expected_cache_key, + "title_html", "description_html", "cached_markdown_version") + end + + expect(results).to eq(expected_results) + end + + it "assigns the values" do + thing.refresh_markdown_cache! + + expect(thing.title_html).to eq('`Hello`') + expect(thing.description_html).to eq("

World

") + expect(thing.cached_markdown_version).to eq(cache_version) + end + end +end diff --git a/spec/lib/gitlab/markdown_cache/redis/store_spec.rb b/spec/lib/gitlab/markdown_cache/redis/store_spec.rb new file mode 100644 index 00000000000..59c038cfb2f --- /dev/null +++ b/spec/lib/gitlab/markdown_cache/redis/store_spec.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe Gitlab::MarkdownCache::Redis::Store, :clean_gitlab_redis_cache do + let(:storable_class) do + Class.new do + cattr_reader :cached_markdown_fields do + Gitlab::MarkdownCache::FieldData.new.tap do |field_data| + field_data[:field_1] = {} + field_data[:field_2] = {} + end + end + + attr_accessor :field_1, :field_2, :field_1_html, :field_2_html, :cached_markdown_version + + def id + 'test-redisbacked-store' + end + end + end + let(:storable) { storable_class.new } + let(:cache_key) { "markdown_cache:#{storable_class}:#{storable.id}" } + + subject(:store) { described_class.new(storable) } + + def read_values + Gitlab::Redis::Cache.with do |r| + r.mapped_hmget(cache_key, + :field_1_html, :field_2_html, :cached_markdown_version) + end + end + + def store_values(values) + Gitlab::Redis::Cache.with do |r| + r.mapped_hmset(cache_key, + values) + end + end + + describe '#save' do + it 'stores updates to html fields and version' do + values_to_store = { field_1_html: "hello", field_2_html: "world", cached_markdown_version: 1 } + + store.save(values_to_store) + + expect(read_values) + .to eq({ field_1_html: "hello", field_2_html: "world", cached_markdown_version: "1" }) + end + end + + describe '#read' do + it 'reads the html fields and version from redis if they were stored' do + stored_values = { field_1_html: "hello", field_2_html: "world", cached_markdown_version: 1 } + + store_values(stored_values) + + expect(store.read.symbolize_keys).to eq(stored_values) + end + + it 'is mared loaded after reading' do + expect(store).not_to be_loaded + + store.read + + expect(store).to be_loaded + end + end +end diff --git a/spec/models/concerns/cache_markdown_field_spec.rb b/spec/models/concerns/cache_markdown_field_spec.rb index 78637ff10c6..52f8b052ad4 100644 --- a/spec/models/concerns/cache_markdown_field_spec.rb +++ b/spec/models/concerns/cache_markdown_field_spec.rb @@ -2,383 +2,213 @@ require 'spec_helper' -describe CacheMarkdownField do - # 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 +describe CacheMarkdownField, :clean_gitlab_redis_cache do + let(:ar_class) do + Class.new(ActiveRecord::Base) do + self.table_name = 'issues' + include CacheMarkdownField + cache_markdown_field :title, pipeline: :single_line + cache_markdown_field :description end + end - extend ActiveModel::Callbacks - define_model_callbacks :create, :update - - include CacheMarkdownField - cache_markdown_field :foo - cache_markdown_field :baz, pipeline: :single_line - cache_markdown_field :zoo, whitelisted: true + let(:other_class) do + Class.new do + include CacheMarkdownField - def self.add_attr(name) - self.attribute_names += [name] - define_attribute_methods(name) - attr_reader(name) - define_method("#{name}=") do |value| - write_attribute(name, value) + def initialize(args = {}) + @title, @description, @cached_markdown_version = args[:title], args[:description], args[:cached_markdown_version] + @title_html, @description_html = args[:title_html], args[:description_html] + @author, @project = args[:author], args[:project] end - end - add_attr :cached_markdown_version + attr_accessor :title, :description, :cached_markdown_version - [:foo, :foo_html, :bar, :baz, :baz_html, :zoo, :zoo_html].each do |name| - add_attr(name) - end - - def initialize(*) - super - - # Pretend new is load - clear_changes_information - end - - def read_attribute(name) - instance_variable_get("@#{name}") - end - - def write_attribute(name, value) - send("#{name}_will_change!") unless value == read_attribute(name) - instance_variable_set("@#{name}", value) - end + cache_markdown_field :title, pipeline: :single_line + cache_markdown_field :description - def save - run_callbacks :update do - changes_applied + def id + "test-markdown-cache" end end - - def has_attribute?(attr_name) - attribute_names.include?(attr_name) - end - end - - def thing_subclass(new_attr) - Class.new(ThingWithMarkdownFields) { add_attr(new_attr) } end let(:markdown) { '`Foo`' } - let(:html) { '

Foo

' } + let(:html) { '

Foo

' } let(:updated_markdown) { '`Bar`' } - let(:updated_html) { '

Bar

' } - - let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: cache_version) } - let(:cache_version) { CacheMarkdownField::CACHE_COMMONMARK_VERSION << 16 } - - before do - stub_commonmark_sourcepos_disabled - end + let(:updated_html) { '

Bar

' } - describe '.attributes' do - it 'excludes cache attributes that is blacklisted by default' do - expect(thing.attributes.keys.sort).to eq(%w[bar baz cached_markdown_version foo zoo zoo_html]) - end - end - - context 'an unchanged markdown field' do - before do - thing.foo = thing.foo - thing.save - end + let(:cache_version) { Gitlab::MarkdownCache::CACHE_COMMONMARK_VERSION << 16 } - it { expect(thing.foo).to eq(markdown) } - it { expect(thing.foo_html).to eq(html) } - it { expect(thing.foo_html_changed?).not_to be_truthy } - it { expect(thing.cached_markdown_version).to eq(cache_version) } + def thing_subclass(klass, extra_attribute) + Class.new(klass) { attr_accessor(extra_attribute) } end - context 'a changed markdown field' do - let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: cache_version - 1) } + shared_examples 'a class with cached markdown fields' do + describe '#cached_html_up_to_date?' do + let(:thing) { klass.new(title: markdown, title_html: html, cached_markdown_version: cache_version) } - before do - thing.foo = updated_markdown - thing.save - end + subject { thing.cached_html_up_to_date?(:title) } - it { expect(thing.foo_html).to eq(updated_html) } - it { expect(thing.cached_markdown_version).to eq(cache_version) } - end + it 'returns false when the version is absent' do + thing.cached_markdown_version = nil - context 'when a markdown field is set repeatedly to an empty string' do - it do - expect(thing).to receive(:refresh_markdown_cache).once - thing.foo = '' - thing.save - thing.foo = '' - thing.save - end - end - - context 'when a markdown field is set repeatedly to a string which renders as empty html' do - it do - expect(thing).to receive(:refresh_markdown_cache).once - thing.foo = '[//]: # (This is also a comment.)' - thing.save - thing.foo = '[//]: # (This is also a comment.)' - thing.save - end - end - - context 'when a markdown field and html field are both changed' do - it do - expect(thing).not_to receive(:refresh_markdown_cache) - thing.foo = '_look over there!_' - thing.foo_html = 'look over there!' - thing.save - end - end - - context 'a non-markdown field changed' do - let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: cache_version - 1) } - - before do - thing.bar = 'OK' - thing.save - end - - it { expect(thing.bar).to eq('OK') } - it { expect(thing.foo).to eq(markdown) } - it { expect(thing.foo_html).to eq(html) } - it { expect(thing.cached_markdown_version).to eq(cache_version) } - end - - context 'version is out of date' do - let(:thing) { ThingWithMarkdownFields.new(foo: updated_markdown, foo_html: html, cached_markdown_version: nil) } - - before do - thing.save - end - - it { expect(thing.foo_html).to eq(updated_html) } - it { expect(thing.cached_markdown_version).to eq(cache_version) } - end - - describe '#cached_html_up_to_date?' do - let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: cache_version) } - - subject { thing.cached_html_up_to_date?(:foo) } - - it 'returns false when the version is absent' do - thing.cached_markdown_version = nil - - is_expected.to be_falsy - end - - it 'returns false when the cached version is too old' do - thing.cached_markdown_version = cache_version - 1 - - is_expected.to be_falsy - end - - it 'returns false when the cached version is in future' do - thing.cached_markdown_version = cache_version + 1 - - is_expected.to be_falsy - end - - it 'returns false when the local version was bumped' do - allow(Gitlab::CurrentSettings.current_application_settings).to receive(:local_markdown_version).and_return(2) - thing.cached_markdown_version = cache_version - - is_expected.to be_falsy - end + is_expected.to be_falsy + end - it 'returns true when the local version is default' do - thing.cached_markdown_version = cache_version + it 'returns false when the version is too early' do + thing.cached_markdown_version -= 1 - is_expected.to be_truthy - end + is_expected.to be_falsy + end - it 'returns true when the cached version is just right' do - allow(Gitlab::CurrentSettings.current_application_settings).to receive(:local_markdown_version).and_return(2) - thing.cached_markdown_version = cache_version + 2 + it 'returns false when the version is too late' do + thing.cached_markdown_version += 1 - is_expected.to be_truthy - end + is_expected.to be_falsy + end - it 'returns false if markdown has been changed but html has not' do - thing.foo = updated_html + it 'returns false when the local version was bumped' do + allow(Gitlab::CurrentSettings.current_application_settings).to receive(:local_markdown_version).and_return(2) + thing.cached_markdown_version = cache_version - is_expected.to be_falsy - end + is_expected.to be_falsy + end - it 'returns true if markdown has not been changed but html has' do - thing.foo_html = updated_html + it 'returns true when the local version is default' do + thing.cached_markdown_version = cache_version - is_expected.to be_truthy - end + is_expected.to be_truthy + end - it 'returns true if markdown and html have both been changed' do - thing.foo = updated_markdown - thing.foo_html = updated_html + it 'returns true when the cached version is just right' do + allow(Gitlab::CurrentSettings.current_application_settings).to receive(:local_markdown_version).and_return(2) + thing.cached_markdown_version = cache_version + 2 - is_expected.to be_truthy + is_expected.to be_truthy + end end - it 'returns false if the markdown field is set but the html is not' do - thing.foo_html = nil + describe '#latest_cached_markdown_version' do + let(:thing) { klass.new } + subject { thing.latest_cached_markdown_version } - is_expected.to be_falsy + it 'returns default version' do + thing.cached_markdown_version = nil + is_expected.to eq(cache_version) + end end - end - - describe '#latest_cached_markdown_version' do - subject { thing.latest_cached_markdown_version } - it 'returns default version' do - thing.cached_markdown_version = nil - is_expected.to eq(cache_version) - end - end + describe '#refresh_markdown_cache' do + let(:thing) { klass.new(description: markdown, description_html: html, cached_markdown_version: cache_version) } - describe '#refresh_markdown_cache' do - before do - thing.foo = updated_markdown - end + before do + thing.description = updated_markdown + end - it 'fills all html fields' do - thing.refresh_markdown_cache + it 'fills all html fields' do + thing.refresh_markdown_cache - expect(thing.foo_html).to eq(updated_html) - expect(thing.foo_html_changed?).to be_truthy - expect(thing.baz_html_changed?).to be_truthy - end + expect(thing.description_html).to eq(updated_html) + end - it 'does not save the result' do - expect(thing).not_to receive(:update_columns) + it 'does not save the result' do + expect(thing).not_to receive(:save_markdown) - thing.refresh_markdown_cache - end + thing.refresh_markdown_cache + end - it 'updates the markdown cache version' do - thing.cached_markdown_version = nil - thing.refresh_markdown_cache + it 'updates the markdown cache version' do + thing.cached_markdown_version = nil + thing.refresh_markdown_cache - expect(thing.cached_markdown_version).to eq(cache_version) + expect(thing.cached_markdown_version).to eq(cache_version) + end end - end - - describe '#refresh_markdown_cache!' do - let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: cache_version) } - before do - thing.foo = updated_markdown - end + describe '#refresh_markdown_cache!' do + let(:thing) { klass.new(description: markdown, description_html: html, cached_markdown_version: cache_version) } - it 'fills all html fields' do - thing.refresh_markdown_cache! + before do + thing.description = updated_markdown + end - expect(thing.foo_html).to eq(updated_html) - expect(thing.foo_html_changed?).to be_truthy - expect(thing.baz_html_changed?).to be_truthy - end + it 'fills all html fields' do + thing.refresh_markdown_cache! - it 'skips saving if not persisted' do - expect(thing).to receive(:persisted?).and_return(false) - expect(thing).not_to receive(:update_columns) + expect(thing.description_html).to eq(updated_html) + end - thing.refresh_markdown_cache! - end + it 'saves the changes' do + expect(thing) + .to receive(:save_markdown) + .with("description_html" => updated_html, "title_html" => "", "cached_markdown_version" => cache_version) - it 'saves the changes using #update_columns' do - expect(thing).to receive(:persisted?).and_return(true) - expect(thing).to receive(:update_columns) - .with( - "foo_html" => updated_html, - "baz_html" => "", - "zoo_html" => "", - "cached_markdown_version" => cache_version - ) - - thing.refresh_markdown_cache! + thing.refresh_markdown_cache! + end end - end - describe '#banzai_render_context' do - subject(:context) { thing.banzai_render_context(:foo) } + describe '#banzai_render_context' do + let(:thing) { klass.new(title: markdown, title_html: html, cached_markdown_version: cache_version) } + subject(:context) { thing.banzai_render_context(:title) } - it 'sets project to nil if the object lacks a project' do - is_expected.to have_key(:project) - expect(context[:project]).to be_nil - end + it 'sets project to nil if the object lacks a project' do + is_expected.to have_key(:project) + expect(context[:project]).to be_nil + end - it 'excludes author if the object lacks an author' do - is_expected.not_to have_key(:author) - end + it 'excludes author if the object lacks an author' do + is_expected.not_to have_key(:author) + end - it 'raises if the context for an unrecognised field is requested' do - expect { thing.banzai_render_context(:not_found) }.to raise_error(ArgumentError) - end + it 'raises if the context for an unrecognised field is requested' do + expect { thing.banzai_render_context(:not_found) }.to raise_error(ArgumentError) + end - it 'includes the pipeline' do - baz = thing.banzai_render_context(:baz) + it 'includes the pipeline' do + title_context = thing.banzai_render_context(:title) - expect(baz[:pipeline]).to eq(:single_line) - end + expect(title_context[:pipeline]).to eq(:single_line) + end - it 'returns copies of the context template' do - template = thing.cached_markdown_fields[:baz] - copy = thing.banzai_render_context(:baz) + it 'returns copies of the context template' do + template = thing.cached_markdown_fields[:description] + copy = thing.banzai_render_context(:description) - expect(copy).not_to be(template) - end + expect(copy).not_to be(template) + end - context 'with a project' do - let(:project) { create(:project, group: create(:group)) } - let(:thing) { thing_subclass(:project).new(foo: markdown, foo_html: html, project: project) } + context 'with a project' do + let(:project) { build(:project, group: create(:group)) } + let(:thing) { thing_subclass(klass, :project).new(title: markdown, title_html: html, project: project) } - it 'sets the project in the context' do - is_expected.to have_key(:project) - expect(context[:project]).to eq(project) + it 'sets the project in the context' do + is_expected.to have_key(:project) + expect(context[:project]).to eq(project) + end end - it 'invalidates the cache when project changes' do - thing.project = :new_project - allow(Banzai::Renderer).to receive(:cacheless_render_field).and_return(updated_html) - - thing.save + context 'with an author' do + let(:thing) { thing_subclass(klass, :author).new(title: markdown, title_html: html, author: :author_value) } - expect(thing.foo_html).to eq(updated_html) - expect(thing.baz_html).to eq(updated_html) - expect(thing.cached_markdown_version).to eq(cache_version) + it 'sets the author in the context' do + is_expected.to have_key(:author) + expect(context[:author]).to eq(:author_value) + end end end + end - context 'with an author' do - let(:thing) { thing_subclass(:author).new(foo: markdown, foo_html: html, author: :author_value) } - - it 'sets the author in the context' do - is_expected.to have_key(:author) - expect(context[:author]).to eq(:author_value) - end + context 'for Active record classes' do + let(:klass) { ar_class } - it 'invalidates the cache when author changes' do - thing.author = :new_author - allow(Banzai::Renderer).to receive(:cacheless_render_field).and_return(updated_html) + it_behaves_like 'a class with cached markdown fields' + end - thing.save + context 'for other classes' do + let(:klass) { other_class } - expect(thing.foo_html).to eq(updated_html) - expect(thing.baz_html).to eq(updated_html) - expect(thing.cached_markdown_version).to eq(cache_version) - end - end + it_behaves_like 'a class with cached markdown fields' end end diff --git a/spec/models/resource_label_event_spec.rb b/spec/models/resource_label_event_spec.rb index 7eeb2fae57d..cb52f154299 100644 --- a/spec/models/resource_label_event_spec.rb +++ b/spec/models/resource_label_event_spec.rb @@ -82,13 +82,13 @@ RSpec.describe ResourceLabelEvent, type: :model do end it 'returns true if markdown is outdated' do - subject.attributes = { cached_markdown_version: ((CacheMarkdownField::CACHE_COMMONMARK_VERSION - 1) << 16) | 0 } + subject.attributes = { cached_markdown_version: ((Gitlab::MarkdownCache::CACHE_COMMONMARK_VERSION - 1) << 16) | 0 } expect(subject.outdated_markdown?).to be true end it 'returns false if label and reference are set' do - subject.attributes = { reference: 'whatever', cached_markdown_version: CacheMarkdownField::CACHE_COMMONMARK_VERSION << 16 } + subject.attributes = { reference: 'whatever', cached_markdown_version: Gitlab::MarkdownCache::CACHE_COMMONMARK_VERSION << 16 } expect(subject.outdated_markdown?).to be false end -- cgit v1.2.1 From 56d52340da0f8f15179b83f1206544a2590c22ff Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Wed, 5 Jun 2019 13:50:37 +0800 Subject: Use #cache_key of subject for generated redis key This commit also includes some changes in specs to use `Class.new` approach. --- lib/gitlab/markdown_cache/redis/store.rb | 2 +- .../markdown_cache/active_record/extension_spec.rb | 26 ++++++++++-------- .../gitlab/markdown_cache/redis/extension_spec.rb | 32 ++++++++++++---------- spec/lib/gitlab/markdown_cache/redis/store_spec.rb | 6 +++- spec/models/concerns/cache_markdown_field_spec.rb | 4 +++ 5 files changed, 42 insertions(+), 28 deletions(-) diff --git a/lib/gitlab/markdown_cache/redis/store.rb b/lib/gitlab/markdown_cache/redis/store.rb index a35eebc6a1b..2d4a89d9f1c 100644 --- a/lib/gitlab/markdown_cache/redis/store.rb +++ b/lib/gitlab/markdown_cache/redis/store.rb @@ -48,7 +48,7 @@ module Gitlab "This class has no id to use for caching" end - "markdown_cache:#{@subject.class}:#{@subject.id}" + "markdown_cache:#{@subject.cache_key}" end end end diff --git a/spec/lib/gitlab/markdown_cache/active_record/extension_spec.rb b/spec/lib/gitlab/markdown_cache/active_record/extension_spec.rb index 6700b53e790..18052b1991c 100644 --- a/spec/lib/gitlab/markdown_cache/active_record/extension_spec.rb +++ b/spec/lib/gitlab/markdown_cache/active_record/extension_spec.rb @@ -2,17 +2,19 @@ require 'spec_helper' describe Gitlab::MarkdownCache::ActiveRecord::Extension do - class ARThingWithMarkdownFields < ActiveRecord::Base - self.table_name = 'issues' - include CacheMarkdownField - cache_markdown_field :title, whitelisted: true - cache_markdown_field :description, pipeline: :single_line + let(:klass) do + Class.new(ActiveRecord::Base) do + self.table_name = 'issues' + include CacheMarkdownField + cache_markdown_field :title, whitelisted: true + cache_markdown_field :description, pipeline: :single_line - attr_accessor :author, :project + attr_accessor :author, :project + end end let(:cache_version) { Gitlab::MarkdownCache::CACHE_COMMONMARK_VERSION << 16 } - let(:thing) { ARThingWithMarkdownFields.new(title: markdown, title_html: html, cached_markdown_version: cache_version) } + let(:thing) { klass.new(title: markdown, title_html: html, cached_markdown_version: cache_version) } let(:markdown) { '`Foo`' } let(:html) { '

Foo

' } @@ -21,7 +23,7 @@ describe Gitlab::MarkdownCache::ActiveRecord::Extension do let(:updated_html) { '

Bar

' } context 'an unchanged markdown field' do - let(:thing) { ARThingWithMarkdownFields.new(title: markdown) } + let(:thing) { klass.new(title: markdown) } before do thing.title = thing.title @@ -35,7 +37,7 @@ describe Gitlab::MarkdownCache::ActiveRecord::Extension do end context 'a changed markdown field' do - let(:thing) { ARThingWithMarkdownFields.new(title: markdown, title_html: html, cached_markdown_version: cache_version) } + let(:thing) { klass.new(title: markdown, title_html: html, cached_markdown_version: cache_version) } before do thing.title = updated_markdown @@ -67,7 +69,7 @@ describe Gitlab::MarkdownCache::ActiveRecord::Extension do end context 'a non-markdown field changed' do - let(:thing) { ARThingWithMarkdownFields.new(title: markdown, title_html: html, cached_markdown_version: cache_version) } + let(:thing) { klass.new(title: markdown, title_html: html, cached_markdown_version: cache_version) } before do thing.state = 'closed' @@ -81,7 +83,7 @@ describe Gitlab::MarkdownCache::ActiveRecord::Extension do end context 'version is out of date' do - let(:thing) { ARThingWithMarkdownFields.new(title: updated_markdown, title_html: html, cached_markdown_version: nil) } + let(:thing) { klass.new(title: updated_markdown, title_html: html, cached_markdown_version: nil) } before do thing.save @@ -122,7 +124,7 @@ describe Gitlab::MarkdownCache::ActiveRecord::Extension do end describe '#cached_html_up_to_date?' do - let(:thing) { ARThingWithMarkdownFields.create(title: updated_markdown, title_html: html, cached_markdown_version: nil) } + let(:thing) { klass.create(title: updated_markdown, title_html: html, cached_markdown_version: nil) } subject { thing.cached_html_up_to_date?(:title) } it 'returns false if markdown has been changed but html has not' do diff --git a/spec/lib/gitlab/markdown_cache/redis/extension_spec.rb b/spec/lib/gitlab/markdown_cache/redis/extension_spec.rb index d3d3cd6f03c..24cfb399cc3 100644 --- a/spec/lib/gitlab/markdown_cache/redis/extension_spec.rb +++ b/spec/lib/gitlab/markdown_cache/redis/extension_spec.rb @@ -2,30 +2,34 @@ require 'spec_helper' describe Gitlab::MarkdownCache::Redis::Extension, :clean_gitlab_redis_cache do - class ThingWithMarkdownFields - include CacheMarkdownField + let(:klass) do + Class.new do + include CacheMarkdownField - def initialize(title: nil, description: nil) - @title, @description = title, description - end + def initialize(title: nil, description: nil) + @title, @description = title, description + end + + attr_reader :title, :description - attr_reader :title, :description + cache_markdown_field :title, pipeline: :single_line + cache_markdown_field :description - cache_markdown_field :title, pipeline: :single_line - cache_markdown_field :description + def id + "test-markdown-cache" + end - def id - "test-markdown-cache" + def cache_key + "cache-key" + end end end let(:cache_version) { Gitlab::MarkdownCache::CACHE_COMMONMARK_VERSION << 16 } - let(:thing) { ThingWithMarkdownFields.new(title: "`Hello`", description: "`World`") } - let(:expected_cache_key) { "markdown_cache:ThingWithMarkdownFields:test-markdown-cache" } + let(:thing) { klass.new(title: "`Hello`", description: "`World`") } + let(:expected_cache_key) { "markdown_cache:cache-key" } it 'defines the html attributes' do - thing = ThingWithMarkdownFields.new - expect(thing).to respond_to(:title_html, :description_html, :cached_markdown_version) end diff --git a/spec/lib/gitlab/markdown_cache/redis/store_spec.rb b/spec/lib/gitlab/markdown_cache/redis/store_spec.rb index 59c038cfb2f..e7701cf1306 100644 --- a/spec/lib/gitlab/markdown_cache/redis/store_spec.rb +++ b/spec/lib/gitlab/markdown_cache/redis/store_spec.rb @@ -16,10 +16,14 @@ describe Gitlab::MarkdownCache::Redis::Store, :clean_gitlab_redis_cache do def id 'test-redisbacked-store' end + + def cache_key + "cache-key" + end end end let(:storable) { storable_class.new } - let(:cache_key) { "markdown_cache:#{storable_class}:#{storable.id}" } + let(:cache_key) { "markdown_cache:#{storable.cache_key}" } subject(:store) { described_class.new(storable) } diff --git a/spec/models/concerns/cache_markdown_field_spec.rb b/spec/models/concerns/cache_markdown_field_spec.rb index 52f8b052ad4..9e6b5d56805 100644 --- a/spec/models/concerns/cache_markdown_field_spec.rb +++ b/spec/models/concerns/cache_markdown_field_spec.rb @@ -30,6 +30,10 @@ describe CacheMarkdownField, :clean_gitlab_redis_cache do def id "test-markdown-cache" end + + def cache_key + "cache-key" + end end end -- cgit v1.2.1 From de21320db21e63db59cbba870ac23624a298214d Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Wed, 5 Jun 2019 20:06:37 +0800 Subject: Remove requirement for id for #markdown_cache_key It's not needed anymore as we require `#cache_key` instead. --- lib/gitlab/markdown_cache/redis/store.rb | 4 ++-- spec/lib/gitlab/markdown_cache/redis/extension_spec.rb | 4 ---- spec/lib/gitlab/markdown_cache/redis/store_spec.rb | 4 ---- spec/models/concerns/cache_markdown_field_spec.rb | 4 ---- 4 files changed, 2 insertions(+), 14 deletions(-) diff --git a/lib/gitlab/markdown_cache/redis/store.rb b/lib/gitlab/markdown_cache/redis/store.rb index 2d4a89d9f1c..0f954404808 100644 --- a/lib/gitlab/markdown_cache/redis/store.rb +++ b/lib/gitlab/markdown_cache/redis/store.rb @@ -43,9 +43,9 @@ module Gitlab end def markdown_cache_key - unless @subject.respond_to?(:id) + unless @subject.respond_to?(:cache_key) raise Gitlab::MarkdownCache::UnsupportedClassError, - "This class has no id to use for caching" + "This class has no cache_key to use for caching" end "markdown_cache:#{@subject.cache_key}" diff --git a/spec/lib/gitlab/markdown_cache/redis/extension_spec.rb b/spec/lib/gitlab/markdown_cache/redis/extension_spec.rb index 24cfb399cc3..b6a781de426 100644 --- a/spec/lib/gitlab/markdown_cache/redis/extension_spec.rb +++ b/spec/lib/gitlab/markdown_cache/redis/extension_spec.rb @@ -15,10 +15,6 @@ describe Gitlab::MarkdownCache::Redis::Extension, :clean_gitlab_redis_cache do cache_markdown_field :title, pipeline: :single_line cache_markdown_field :description - def id - "test-markdown-cache" - end - def cache_key "cache-key" end diff --git a/spec/lib/gitlab/markdown_cache/redis/store_spec.rb b/spec/lib/gitlab/markdown_cache/redis/store_spec.rb index e7701cf1306..95c68e7d491 100644 --- a/spec/lib/gitlab/markdown_cache/redis/store_spec.rb +++ b/spec/lib/gitlab/markdown_cache/redis/store_spec.rb @@ -13,10 +13,6 @@ describe Gitlab::MarkdownCache::Redis::Store, :clean_gitlab_redis_cache do attr_accessor :field_1, :field_2, :field_1_html, :field_2_html, :cached_markdown_version - def id - 'test-redisbacked-store' - end - def cache_key "cache-key" end diff --git a/spec/models/concerns/cache_markdown_field_spec.rb b/spec/models/concerns/cache_markdown_field_spec.rb index 9e6b5d56805..0e5fb2b5153 100644 --- a/spec/models/concerns/cache_markdown_field_spec.rb +++ b/spec/models/concerns/cache_markdown_field_spec.rb @@ -27,10 +27,6 @@ describe CacheMarkdownField, :clean_gitlab_redis_cache do cache_markdown_field :title, pipeline: :single_line cache_markdown_field :description - def id - "test-markdown-cache" - end - def cache_key "cache-key" end -- cgit v1.2.1 From 93dd5390b6b4bc94cc83626e1369f3925c82028e Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Wed, 5 Jun 2019 20:09:50 +0800 Subject: Use markdown_field helper to display full_title --- app/views/projects/tree/_tree_commit_column.html.haml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/views/projects/tree/_tree_commit_column.html.haml b/app/views/projects/tree/_tree_commit_column.html.haml index e37fd7624be..065fef606d5 100644 --- a/app/views/projects/tree/_tree_commit_column.html.haml +++ b/app/views/projects/tree/_tree_commit_column.html.haml @@ -1,2 +1,3 @@ +- full_title = markdown_field(commit, :full_title) %span.str-truncated - = link_to_html commit.redacted_full_title_html, project_commit_path(@project, commit.id), title: commit.redacted_full_title_html, class: 'tree-commit-link' + = link_to_html full_title, project_commit_path(@project, commit.id), title: full_title, class: 'tree-commit-link' -- cgit v1.2.1 From ea12c5aae83dd3f2edefce765438e94ff7c4f870 Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Wed, 5 Jun 2019 20:26:56 +0800 Subject: Cleanup #attributes method Since we're prepending the ActiveRecord::Extension module, we can take advantage of it and avoid using an alias to extend the original #attributes method. --- .../markdown_cache/active_record/extension.rb | 34 +++++++++------------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/lib/gitlab/markdown_cache/active_record/extension.rb b/lib/gitlab/markdown_cache/active_record/extension.rb index bcc3432bd31..f3abe631779 100644 --- a/lib/gitlab/markdown_cache/active_record/extension.rb +++ b/lib/gitlab/markdown_cache/active_record/extension.rb @@ -7,31 +7,25 @@ module Gitlab extend ActiveSupport::Concern included do - # 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 - html_fields = cached_markdown_fields.html_fields - whitelisted = cached_markdown_fields.html_fields_whitelisted - exclude_fields = html_fields - whitelisted - - exclude_fields.each do |field| - attrs.delete(field) - end - - if whitelisted.empty? - attrs.delete('cached_markdown_version') - end - - attrs - end - # Using before_update here conflicts with elasticsearch-model somehow before_create :refresh_markdown_cache, if: :invalidated_markdown_cache? before_update :refresh_markdown_cache, if: :invalidated_markdown_cache? end + # Always exclude _html fields from attributes (including serialization). + # They contain unredacted HTML, which would be a security issue + def attributes + attrs = super + html_fields = cached_markdown_fields.html_fields + whitelisted = cached_markdown_fields.html_fields_whitelisted + exclude_fields = html_fields - whitelisted + + attrs.except!(*exclude_fields) + attrs.delete('cached_markdown_version') if whitelisted.empty? + + attrs + end + def changed_markdown_fields changed_attributes.keys.map(&:to_s) & cached_markdown_fields.markdown_fields.map(&:to_s) end -- cgit v1.2.1