Commit 4eecfba1 authored by Vijay Hawoldar's avatar Vijay Hawoldar

Conditionally cache Snippet content

Not all Snippets contain Markdown content and so
the `content` field should not always be cached.

This change will attempt to determine if the content
is Markdown based on the Snippet filename and only cache
if it is
parent 54cd52b6
...@@ -20,6 +20,10 @@ module CacheMarkdownField ...@@ -20,6 +20,10 @@ module CacheMarkdownField
false false
end end
def can_cache_field?(field)
true
end
# Returns the default Banzai render context for the cached markdown field. # Returns the default Banzai render context for the cached markdown field.
def banzai_render_context(field) def banzai_render_context(field)
raise ArgumentError.new("Unknown field: #{field.inspect}") unless raise ArgumentError.new("Unknown field: #{field.inspect}") unless
...@@ -38,17 +42,23 @@ module CacheMarkdownField ...@@ -38,17 +42,23 @@ module CacheMarkdownField
context context
end end
# Update every column in a row if any one is invalidated, as we only store def rendered_field_content(markdown_field)
# one version per row return unless can_cache_field?(markdown_field)
def refresh_markdown_cache
options = { skip_project_check: skip_project_check? } options = { skip_project_check: skip_project_check? }
Banzai::Renderer.cacheless_render_field(self, markdown_field, options)
end
# Update every applicable column in a row if any one is invalidated, as we only store
# one version per row
def refresh_markdown_cache
updates = cached_markdown_fields.markdown_fields.map do |markdown_field| updates = cached_markdown_fields.markdown_fields.map do |markdown_field|
[ [
cached_markdown_fields.html_field(markdown_field), cached_markdown_fields.html_field(markdown_field),
Banzai::Renderer.cacheless_render_field(self, markdown_field, options) rendered_field_content(markdown_field)
] ]
end.to_h end.to_h
updates['cached_markdown_version'] = latest_cached_markdown_version updates['cached_markdown_version'] = latest_cached_markdown_version
updates.each { |field, data| write_markdown_field(field, data) } updates.each { |field, data| write_markdown_field(field, data) }
......
...@@ -301,6 +301,10 @@ class Snippet < ApplicationRecord ...@@ -301,6 +301,10 @@ class Snippet < ApplicationRecord
repository.update!(shard_name: repository_storage, disk_path: disk_path) repository.update!(shard_name: repository_storage, disk_path: disk_path)
end end
def can_cache_field?(field)
field != :content || MarkupHelper.gitlab_markdown?(file_name)
end
class << self class << self
# Searches for snippets with a matching title or file name. # Searches for snippets with a matching title or file name.
# #
......
---
title: Fix Snippet content incorrectly caching
merge_request: 25985
author:
type: fixed
...@@ -230,6 +230,26 @@ describe CacheMarkdownField, :clean_gitlab_redis_cache do ...@@ -230,6 +230,26 @@ describe CacheMarkdownField, :clean_gitlab_redis_cache do
end end
end end
end end
describe '#rendered_field_content' do
let(:thing) { klass.new(description: markdown, description_html: nil, cached_markdown_version: cache_version) }
context 'when a field can be cached' do
it 'returns the html' do
thing.description = updated_markdown
expect(thing.rendered_field_content(:description)).to eq updated_html
end
end
context 'when a field cannot be cached' do
it 'returns nil' do
allow(thing).to receive(:can_cache_field?).with(:description).and_return false
expect(thing.rendered_field_content(:description)).to eq nil
end
end
end
end end
context 'for Active record classes' do context 'for Active record classes' do
......
...@@ -631,4 +631,26 @@ describe Snippet do ...@@ -631,4 +631,26 @@ describe Snippet do
end end
end end
end end
describe '#can_cache_field?' do
using RSpec::Parameterized::TableSyntax
let(:snippet) { create(:snippet, file_name: file_name) }
subject { snippet.can_cache_field?(field) }
where(:field, :file_name, :result) do
:title | nil | true
:title | 'foo.bar' | true
:description | nil | true
:description | 'foo.bar' | true
:content | nil | false
:content | 'bar.foo' | false
:content | 'markdown.md' | true
end
with_them do
it { is_expected.to eq result }
end
end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment