diff options
| -rw-r--r-- | app/models/concerns/cache_markdown_field.rb | 126 | ||||
| -rw-r--r-- | app/workers/clear_database_cache_worker.rb | 24 | ||||
| -rw-r--r-- | changelogs/unreleased/30672-versioned-markdown-cache.yml | 4 | ||||
| -rw-r--r-- | config/sidekiq_queues.yml | 1 | ||||
| -rw-r--r-- | db/migrate/20170410133135_add_version_field_to_markdown_cache.rb | 25 | ||||
| -rw-r--r-- | db/schema.rb | 13 | ||||
| -rw-r--r-- | lib/banzai/renderer.rb | 41 | ||||
| -rw-r--r-- | lib/gitlab/metrics.rb | 10 | ||||
| -rw-r--r-- | lib/tasks/cache.rake | 7 | ||||
| -rw-r--r-- | spec/lib/banzai/object_renderer_spec.rb | 4 | ||||
| -rw-r--r-- | spec/lib/banzai/renderer_spec.rb | 69 | ||||
| -rw-r--r-- | spec/models/concerns/cache_markdown_field_spec.rb | 270 |
12 files changed, 343 insertions, 251 deletions
diff --git a/app/models/concerns/cache_markdown_field.rb b/app/models/concerns/cache_markdown_field.rb index 8ea95beed79..2eedc143968 100644 --- a/app/models/concerns/cache_markdown_field.rb +++ b/app/models/concerns/cache_markdown_field.rb @@ -8,6 +8,14 @@ # # Corresponding foo_html, bar_html and baz_html fields should exist. module CacheMarkdownField + extend ActiveSupport::Concern + + # Increment this number every time the renderer changes its output + CACHE_VERSION = 1 + + # 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 @@ -30,60 +38,71 @@ module CacheMarkdownField 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 - ].freeze - - def self.caching_classes - CACHING_CLASSES.map(&:constantize) - end - def skip_project_check? false end - extend ActiveSupport::Concern + # 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) - included do - cattr_reader :cached_markdown_fields do - FieldData.new - end + # Always include a project key, or Banzai complains + project = self.project if self.respond_to?(:project) + context = cached_markdown_fields[field].merge(project: project) - # 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) + # Banzai is less strict about authors, so don't always have an author key + context[:author] = self.author if self.respond_to?(:author) - # Always include a project key, or Banzai complains - project = self.project if self.respond_to?(:project) - context = cached_markdown_fields[field].merge(project: project) + context + end - # Banzai is less strict about authors, so don't always have an author key - context[:author] = self.author if self.respond_to?(:author) + # Update every column in a row if any one is invalidated, as we only store + # one version per row + def refresh_markdown_cache!(do_update: false) + options = { skip_project_check: skip_project_check? } - context - end + updates = cached_markdown_fields.markdown_fields.map do |markdown_field| + [ + cached_markdown_fields.html_field(markdown_field), + Banzai::Renderer.cacheless_render_field(self, markdown_field, options) + ] + end.to_h + updates['cached_markdown_version'] = CacheMarkdownField::CACHE_VERSION - # 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) + updates.each {|html_field, data| write_attribute(html_field, data) } - cached_markdown_fields.html_field(field) + update_columns(updates) if persisted? && do_update + end + + def cached_html_up_to_date?(markdown_field) + html_field = cached_markdown_fields.html_field(markdown_field) + + markdown_changed = attribute_changed?(markdown_field) || false + html_changed = attribute_changed?(html_field) || false + + CacheMarkdownField::CACHE_VERSION == cached_markdown_version && + (html_changed || markdown_changed == html_changed) + end + + def invalidated_markdown_cache? + cached_markdown_fields.html_fields.any? {|html_field| attribute_invalidated?(html_field) } + end + + def attribute_invalidated?(attr) + __send__("#{attr}_invalidated?") + end + + def cached_html_for(markdown_field) + raise ArgumentError.new("Unknown field: #{field}") unless + cached_markdown_fields.markdown_fields.include?(markdown_field) + + __send__(cached_markdown_fields.html_field(markdown_field)) + end + + included do + cattr_reader :cached_markdown_fields do + FieldData.new end # Always exclude _html fields from attributes (including serialization). @@ -92,12 +111,16 @@ module CacheMarkdownField def attributes attrs = attributes_before_markdown_cache + attrs.delete('cached_markdown_version') + cached_markdown_fields.html_fields.each do |field| attrs.delete(field) end attrs end + + before_save :refresh_markdown_cache!, if: :invalidated_markdown_cache? end class_methods do @@ -107,31 +130,18 @@ module CacheMarkdownField # 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 - options = { skip_project_check: skip_project_check? } - html = Banzai::Renderer.cacheless_render_field(self, markdown_field, options) - __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? + invalidations = changed_fields & [markdown_field.to_s, *INVALIDATED_BY] + !invalidations.empty? || !cached_html_up_to_date?(markdown_field) end - - before_save cache_method, if: invalidation_method end end end diff --git a/app/workers/clear_database_cache_worker.rb b/app/workers/clear_database_cache_worker.rb deleted file mode 100644 index c4cb4733482..00000000000 --- a/app/workers/clear_database_cache_worker.rb +++ /dev/null @@ -1,24 +0,0 @@ -# This worker clears all cache fields in the database, working in batches. -class ClearDatabaseCacheWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue - - 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/changelogs/unreleased/30672-versioned-markdown-cache.yml b/changelogs/unreleased/30672-versioned-markdown-cache.yml new file mode 100644 index 00000000000..d8f977b01de --- /dev/null +++ b/changelogs/unreleased/30672-versioned-markdown-cache.yml @@ -0,0 +1,4 @@ +--- +title: Replace rake cache:clear:db with an automatic mechanism +merge_request: 10597 +author: diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index bf8964d7f68..c3bd73533d0 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -34,7 +34,6 @@ - [repository_fork, 1] - [repository_import, 1] - [project_service, 1] - - [clear_database_cache, 1] - [delete_user, 1] - [delete_merged_branches, 1] - [authorized_projects, 1] diff --git a/db/migrate/20170410133135_add_version_field_to_markdown_cache.rb b/db/migrate/20170410133135_add_version_field_to_markdown_cache.rb new file mode 100644 index 00000000000..d9209fe5770 --- /dev/null +++ b/db/migrate/20170410133135_add_version_field_to_markdown_cache.rb @@ -0,0 +1,25 @@ +class AddVersionFieldToMarkdownCache < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + %i[ + abuse_reports + appearances + application_settings + broadcast_messages + issues + labels + merge_requests + milestones + namespaces + notes + projects + releases + snippets + ].each do |table| + add_column table, :cached_markdown_version, :integer, limit: 4 + end + end +end diff --git a/db/schema.rb b/db/schema.rb index f89e9adb7d6..290d969d7de 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -24,6 +24,7 @@ ActiveRecord::Schema.define(version: 20170419001229) do t.datetime "created_at" t.datetime "updated_at" t.text "message_html" + t.integer "cached_markdown_version" end create_table "appearances", force: :cascade do |t| @@ -34,6 +35,7 @@ ActiveRecord::Schema.define(version: 20170419001229) do t.datetime "created_at", null: false t.datetime "updated_at", null: false t.text "description_html" + t.integer "cached_markdown_version" end create_table "application_settings", force: :cascade do |t| @@ -116,6 +118,7 @@ ActiveRecord::Schema.define(version: 20170419001229) do t.integer "unique_ips_limit_time_window" t.boolean "unique_ips_limit_enabled", default: false, null: false t.decimal "polling_interval_multiplier", default: 1.0, null: false + t.integer "cached_markdown_version" t.boolean "usage_ping_enabled", default: true, null: false t.string "uuid" end @@ -161,6 +164,7 @@ ActiveRecord::Schema.define(version: 20170419001229) do t.string "color" t.string "font" t.text "message_html" + t.integer "cached_markdown_version" end create_table "chat_names", force: :cascade do |t| @@ -479,6 +483,7 @@ ActiveRecord::Schema.define(version: 20170419001229) do t.integer "time_estimate" t.integer "relative_position" t.datetime "closed_at" + t.integer "cached_markdown_version" end add_index "issues", ["assignee_id"], name: "index_issues_on_assignee_id", using: :btree @@ -543,6 +548,7 @@ ActiveRecord::Schema.define(version: 20170419001229) do t.text "description_html" t.string "type" t.integer "group_id" + t.integer "cached_markdown_version" end add_index "labels", ["group_id", "project_id", "title"], name: "index_labels_on_group_id_and_project_id_and_title", unique: true, using: :btree @@ -663,6 +669,7 @@ ActiveRecord::Schema.define(version: 20170419001229) do t.text "title_html" t.text "description_html" t.integer "time_estimate" + t.integer "cached_markdown_version" end add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree @@ -700,6 +707,7 @@ ActiveRecord::Schema.define(version: 20170419001229) do t.text "title_html" t.text "description_html" t.date "start_date" + t.integer "cached_markdown_version" end add_index "milestones", ["description"], name: "index_milestones_on_description_trigram", using: :gin, opclasses: {"description"=>"gin_trgm_ops"} @@ -726,6 +734,7 @@ ActiveRecord::Schema.define(version: 20170419001229) do t.integer "parent_id" t.boolean "require_two_factor_authentication", default: false, null: false t.integer "two_factor_grace_period", default: 48, null: false + t.integer "cached_markdown_version" end add_index "namespaces", ["created_at"], name: "index_namespaces_on_created_at", using: :btree @@ -760,6 +769,7 @@ ActiveRecord::Schema.define(version: 20170419001229) do t.integer "resolved_by_id" t.string "discussion_id" t.text "note_html" + t.integer "cached_markdown_version" end add_index "notes", ["author_id"], name: "index_notes_on_author_id", using: :btree @@ -956,6 +966,7 @@ ActiveRecord::Schema.define(version: 20170419001229) do t.integer "auto_cancel_pending_pipelines", default: 0, null: false t.boolean "printing_merge_request_link_enabled", default: true, null: false t.string "import_jid" + t.integer "cached_markdown_version" end add_index "projects", ["ci_id"], name: "index_projects_on_ci_id", using: :btree @@ -1028,6 +1039,7 @@ ActiveRecord::Schema.define(version: 20170419001229) do t.datetime "created_at" t.datetime "updated_at" t.text "description_html" + t.integer "cached_markdown_version" end add_index "releases", ["project_id", "tag"], name: "index_releases_on_project_id_and_tag", using: :btree @@ -1099,6 +1111,7 @@ ActiveRecord::Schema.define(version: 20170419001229) do t.integer "visibility_level", default: 0, null: false t.text "title_html" t.text "content_html" + t.integer "cached_markdown_version" end add_index "snippets", ["author_id"], name: "index_snippets_on_author_id", using: :btree diff --git a/lib/banzai/renderer.rb b/lib/banzai/renderer.rb index 74663556cbb..c7801cb5baf 100644 --- a/lib/banzai/renderer.rb +++ b/lib/banzai/renderer.rb @@ -1,7 +1,5 @@ module Banzai module Renderer - module_function - # Convert a Markdown String into an HTML-safe String of HTML # # Note that while the returned HTML will have been sanitized of dangerous @@ -16,7 +14,7 @@ module Banzai # context - Hash of context options passed to our HTML Pipeline # # Returns an HTML-safe String - def render(text, context = {}) + def self.render(text, context = {}) cache_key = context.delete(:cache_key) cache_key = full_cache_key(cache_key, context[:pipeline]) @@ -35,24 +33,16 @@ module Banzai # 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) - update_object(object, html_field, html) unless object.new_record? || object.destroyed? + # The context to use is managed by the object and cannot be changed. + # Use #render, passing it the field text, if a custom rendering is needed. + def self.render_field(object, field) + object.refresh_markdown_cache!(do_update: update_object?(object)) unless object.cached_html_up_to_date?(field) - html + object.cached_html_for(field) end # Same as +render_field+, but without consulting or updating the cache field - def cacheless_render_field(object, field, options = {}) + def self.cacheless_render_field(object, field, options = {}) text = object.__send__(field) context = object.banzai_render_context(field).merge(options) @@ -82,7 +72,7 @@ module Banzai # texts_and_contexts # => [{ text: '### Hello', # context: { cache_key: [note, :note] } }] - def cache_collection_render(texts_and_contexts) + def self.cache_collection_render(texts_and_contexts) items_collection = texts_and_contexts.each_with_index do |item, index| context = item[:context] cache_key = full_cache_multi_key(context.delete(:cache_key), context[:pipeline]) @@ -111,7 +101,7 @@ module Banzai items_collection.map { |item| item[:rendered] } end - def render_result(text, context = {}) + def self.render_result(text, context = {}) text = Pipeline[:pre_process].to_html(text, context) if text Pipeline[context[:pipeline]].call(text, context) @@ -130,7 +120,7 @@ module Banzai # :user - User object # # Returns an HTML-safe String - def post_process(html, context) + def self.post_process(html, context) context = Pipeline[context[:pipeline]].transform_context(context) pipeline = Pipeline[:post_process] @@ -141,7 +131,7 @@ module Banzai end.html_safe end - def cacheless_render(text, context = {}) + def self.cacheless_render(text, context = {}) Gitlab::Metrics.measure(:banzai_cacheless_render) do result = render_result(text, context) @@ -154,7 +144,7 @@ module Banzai end end - def full_cache_key(cache_key, pipeline_name) + def self.full_cache_key(cache_key, pipeline_name) return unless cache_key ["banzai", *cache_key, pipeline_name || :full] end @@ -162,13 +152,14 @@ module Banzai # To map Rails.cache.read_multi results we need to know the Rails.cache.expanded_key. # Other option will be to generate stringified keys on our side and don't delegate to Rails.cache.expanded_key # method. - def full_cache_multi_key(cache_key, pipeline_name) + def self.full_cache_multi_key(cache_key, pipeline_name) return unless cache_key Rails.cache.send(:expanded_key, full_cache_key(cache_key, pipeline_name)) end - def update_object(object, html_field, html) - object.update_column(html_field, html) + # GitLab EE needs to disable updates on GET requests in Geo + def self.update_object?(object) + true end end end diff --git a/lib/gitlab/metrics.rb b/lib/gitlab/metrics.rb index 857e0abf710..c6dfa4ad9bd 100644 --- a/lib/gitlab/metrics.rb +++ b/lib/gitlab/metrics.rb @@ -138,6 +138,11 @@ module Gitlab @series_prefix ||= Sidekiq.server? ? 'sidekiq_' : 'rails_' end + # Allow access from other metrics related middlewares + def self.current_transaction + Transaction.current + end + # When enabled this should be set before being used as the usual pattern # "@foo ||= bar" is _not_ thread-safe. if enabled? @@ -149,10 +154,5 @@ module Gitlab new(udp: { host: host, port: port }) end end - - # Allow access from other metrics related middlewares - def self.current_transaction - Transaction.current - end end end diff --git a/lib/tasks/cache.rake b/lib/tasks/cache.rake index d55923673b1..125a3d560d6 100644 --- a/lib/tasks/cache.rake +++ b/lib/tasks/cache.rake @@ -21,12 +21,7 @@ namespace :cache do end end - desc "GitLab | Clear database cache (in the background)" - task db: :environment do - ClearDatabaseCacheWorker.perform_async - end - - task all: [:db, :redis] + task all: [:redis] end task clear: 'cache:clear:redis' diff --git a/spec/lib/banzai/object_renderer_spec.rb b/spec/lib/banzai/object_renderer_spec.rb index 4817fcd031a..dd2674f9f20 100644 --- a/spec/lib/banzai/object_renderer_spec.rb +++ b/spec/lib/banzai/object_renderer_spec.rb @@ -4,13 +4,13 @@ describe Banzai::ObjectRenderer do let(:project) { create(:empty_project) } let(:user) { project.owner } let(:renderer) { described_class.new(project, user, custom_value: 'value') } - let(:object) { Note.new(note: 'hello', note_html: '<p>hello</p>') } + let(:object) { Note.new(note: 'hello', note_html: '<p dir="auto">hello</p>', cached_markdown_version: CacheMarkdownField::CACHE_VERSION) } describe '#render' do it 'renders and redacts an Array of objects' do renderer.render([object], :note) - expect(object.redacted_note_html).to eq '<p>hello</p>' + expect(object.redacted_note_html).to eq '<p dir="auto">hello</p>' expect(object.user_visible_reference_count).to eq 0 end diff --git a/spec/lib/banzai/renderer_spec.rb b/spec/lib/banzai/renderer_spec.rb index aaa6b12e67e..e6f8d2a1fed 100644 --- a/spec/lib/banzai/renderer_spec.rb +++ b/spec/lib/banzai/renderer_spec.rb @@ -1,73 +1,36 @@ 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 - ) + def fake_object(fresh:) + object = double('object') - 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)) + allow(object).to receive(:cached_html_up_to_date?).with(:field).and_return(fresh) + allow(object).to receive(:cached_html_for).with(:field).and_return('field_html') object end - describe "#render_field" do + describe '#render_field' do let(:renderer) { Banzai::Renderer } - let(:subject) { renderer.render_field(object, :field) } + 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 stale cache' do + let(:object) { fake_object(fresh: false) } - context "with a filled cache" do - let(:object) { fake_object(:markdown, :html) } + it 'caches and returns the result' do + expect(object).to receive(:refresh_markdown_cache!).with(do_update: true) - it "uses the cache" do - expect_render.never - expect_cache_update.never - should eq(:html) + is_expected.to eq('field_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 'with an up-to-date cache' do + let(:object) { fake_object(fresh: true) } - context "destroyed object" do - let(:object) { fake_object(:destroyed, :markdown) } + it 'uses the cache' do + expect(object).to receive(:refresh_markdown_cache!).never - it "doesn't cache the result" do - expect_render - expect_cache_update.never - expect(subject).to eq(:html) + is_expected.to eq('field_html') end end end diff --git a/spec/models/concerns/cache_markdown_field_spec.rb b/spec/models/concerns/cache_markdown_field_spec.rb index 6151d53cd91..de0069bdcac 100644 --- a/spec/models/concerns/cache_markdown_field_spec.rb +++ b/spec/models/concerns/cache_markdown_field_spec.rb @@ -1,9 +1,6 @@ require 'spec_helper' describe CacheMarkdownField do - caching_classes = CacheMarkdownField::CACHING_CLASSES - CacheMarkdownField::CACHING_CLASSES = ["ThingWithMarkdownFields"].freeze - # The minimum necessary ActiveModel to test this concern class ThingWithMarkdownFields include ActiveModel::Model @@ -27,18 +24,19 @@ describe CacheMarkdownField do 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) + 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) end end - [:foo, :foo_html, :bar, :baz, :baz_html].each do |attr_name| - add_attr(attr_name) + add_attr :cached_markdown_version + + [:foo, :foo_html, :bar, :baz, :baz_html].each do |name| + add_attr(name) end def initialize(*) @@ -48,6 +46,15 @@ describe CacheMarkdownField do 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 + def save run_callbacks :save do changes_applied @@ -55,127 +62,236 @@ describe CacheMarkdownField do end end - CacheMarkdownField::CACHING_CLASSES = caching_classes - def thing_subclass(new_attr) Class.new(ThingWithMarkdownFields) { add_attr(new_attr) } end - let(:markdown) { "`Foo`" } - let(:html) { "<p><code>Foo</code></p>" } + let(:markdown) { '`Foo`' } + let(:html) { '<p dir="auto"><code>Foo</code></p>' } - let(:updated_markdown) { "`Bar`" } - let(:updated_html) { "<p dir=\"auto\"><code>Bar</code></p>" } + let(:updated_markdown) { '`Bar`' } + let(:updated_html) { '<p dir="auto"><code>Bar</code></p>' } - subject { ThingWithMarkdownFields.new(foo: markdown, foo_html: html) } + let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: CacheMarkdownField::CACHE_VERSION) } - describe ".attributes" do - it "excludes cache attributes" do - expect(thing_subclass(:qux).new.attributes.keys.sort).to eq(%w[bar baz foo qux]) + describe '.attributes' do + it 'excludes cache attributes' do + expect(thing.attributes.keys.sort).to eq(%w[bar baz foo]) 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) + context 'an unchanged markdown field' do + before do + thing.foo = thing.foo + thing.save end + + 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(CacheMarkdownField::CACHE_VERSION) } end - context "an unchanged markdown field" do + context 'a changed markdown field' do before do - subject.foo = subject.foo - subject.save + thing.foo = updated_markdown + thing.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 } + it { expect(thing.foo_html).to eq(updated_html) } + it { expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_VERSION) } end - context "a changed markdown field" do + context 'a non-markdown field changed' do + 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(CacheMarkdownField::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 - subject.foo = updated_markdown - subject.save + thing.save + end + + it { expect(thing.foo_html).to eq(updated_html) } + it { expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_VERSION) } + end + + describe '#cached_html_up_to_date?' do + 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 version is too early' do + thing.cached_markdown_version -= 1 + + is_expected.to be_falsy + end + + it 'returns false when the version is too late' do + thing.cached_markdown_version += 1 + + is_expected.to be_falsy + end + + it 'returns true when the version is just right' do + thing.cached_markdown_version = CacheMarkdownField::CACHE_VERSION + + is_expected.to be_truthy end - it { expect(subject.foo_html).to eq(updated_html) } + it 'returns false if markdown has been changed but html has not' do + thing.foo = updated_html + + is_expected.to be_falsy + end + + it 'returns true if markdown has not been changed but html has' do + thing.foo_html = updated_html + + 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 + + is_expected.to be_truthy + end end - context "a non-markdown field changed" do + describe '#refresh_markdown_cache!' do before do - subject.bar = "OK" - subject.save + thing.foo = updated_markdown + end + + context 'do_update: false' do + 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 + + it 'does not save the result' do + expect(thing).not_to receive(:update_columns) + + thing.refresh_markdown_cache! + end + + it 'updates the markdown cache version' do + thing.cached_markdown_version = nil + thing.refresh_markdown_cache! + + expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_VERSION) + end end - it { expect(subject.bar).to eq("OK") } - it { expect(subject.foo).to eq(markdown) } - it { expect(subject.foo_html).to eq(html) } + context 'do_update: true' do + it 'fills all html fields' do + thing.refresh_markdown_cache!(do_update: true) + + 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 '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!(do_update: true) + end + + 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" => "", "cached_markdown_version" => CacheMarkdownField::CACHE_VERSION) + + thing.refresh_markdown_cache!(do_update: true) + end + end 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) + subject(:context) { thing.banzai_render_context(:foo) } + + 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 - context = subject.banzai_render_context(:foo) - expect(context).not_to have_key(:author) + 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{subject.banzai_render_context(:not_found)}.to raise_error(ArgumentError) + 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 - context = subject.banzai_render_context(:baz) - expect(context[:pipeline]).to eq(:single_line) + it 'includes the pipeline' do + baz = thing.banzai_render_context(:baz) + + expect(baz[: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) + it 'returns copies of the context template' do + template = thing.cached_markdown_fields[:baz] + copy = thing.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) } + context 'with a project' do + let(:thing) { thing_subclass(:project).new(foo: markdown, foo_html: html, project: :project_value) } - 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) + it 'sets the project in the context' do + is_expected.to have_key(:project) + expect(context[:project]).to eq(:project_value) end - it "invalidates the cache when project changes" do - subject.project = :new_project + it 'invalidates the cache when project changes' do + thing.project = :new_project allow(Banzai::Renderer).to receive(:cacheless_render_field).and_return(updated_html) - subject.save + thing.save - expect(subject.foo_html).to eq(updated_html) - expect(subject.baz_html).to eq(updated_html) + expect(thing.foo_html).to eq(updated_html) + expect(thing.baz_html).to eq(updated_html) + expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_VERSION) end end - context "with an author" do - subject { thing_subclass(:author).new(foo: markdown, foo_html: html, author: :author) } + 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 - context = subject.banzai_render_context(:foo) - expect(context).to have_key(:author) - expect(context[:author]).to eq(:author) + it 'sets the author in the context' do + is_expected.to have_key(:author) + expect(context[:author]).to eq(:author_value) end - it "invalidates the cache when author changes" do - subject.author = :new_author + it 'invalidates the cache when author changes' do + thing.author = :new_author allow(Banzai::Renderer).to receive(:cacheless_render_field).and_return(updated_html) - subject.save + thing.save - expect(subject.foo_html).to eq(updated_html) - expect(subject.baz_html).to eq(updated_html) + expect(thing.foo_html).to eq(updated_html) + expect(thing.baz_html).to eq(updated_html) + expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_VERSION) end end end |