Commit 117c14d0 authored by Patrick Bajao's avatar Patrick Bajao

Handle post processed content scenario for MR discussions cache

We cache MR discussions after post processing. Before this fix, we
aren't considering the post processing result (e.g. reference state
change) which results to stale cache.

This fix adds a `Note#post_processed_cache_key` that is also added
to `Discussion#cache_key`. This way, when the post processing
changes the note's content for a certain user, we will be able to
show updated content.
parent 64a920f2
......@@ -163,16 +163,15 @@ class Discussion
end
def cache_key
# Need this so cache will be invalidated when note within a discussion
# has been deleted.
notes_sha = Digest::SHA1.hexdigest(notes.map(&:id).join(':'))
# Need to use the notes' cache key so cache will be invalidated when note
# within a discussion has been deleted or has different data after post
# processing of content.
notes_sha = Digest::SHA1.hexdigest(notes.map(&:post_processed_cache_key).join(':'))
[
CACHE_VERSION,
notes.last.latest_cached_markdown_version,
id,
notes_sha,
notes.max_by(&:updated_at).updated_at,
resolved_at
].join(':')
end
......
......@@ -586,6 +586,13 @@ class Note < ApplicationRecord
review.present? || !author.can_trigger_notifications?
end
def post_processed_cache_key
cache_key_items = [cache_key]
cache_key_items << Digest::SHA1.hexdigest(redacted_note_html) if redacted_note_html.present?
cache_key_items.join(':')
end
private
# Using this method followed by a call to *save* may result in *ActiveRecord::RecordNotUnique* exception
......
......@@ -129,10 +129,10 @@ RSpec.describe DiffDiscussion do
describe '#cache_key' do
it 'returns the cache key with the position sha' do
notes_sha = Digest::SHA1.hexdigest("#{diff_note.id}")
notes_sha = Digest::SHA1.hexdigest("#{diff_note.post_processed_cache_key}")
position_sha = Digest::SHA1.hexdigest(diff_note.position.to_json)
expect(subject.cache_key).to eq("#{described_class::CACHE_VERSION}:#{diff_note.latest_cached_markdown_version}:#{subject.id}:#{notes_sha}:#{diff_note.updated_at}::#{position_sha}")
expect(subject.cache_key).to eq("#{described_class::CACHE_VERSION}:#{subject.id}:#{notes_sha}::#{position_sha}")
end
end
end
......@@ -53,10 +53,10 @@ RSpec.describe Discussion do
end
describe '#cache_key' do
let(:notes_sha) { Digest::SHA1.hexdigest("#{first_note.id}:#{second_note.id}:#{third_note.id}") }
let(:notes_sha) { Digest::SHA1.hexdigest("#{first_note.post_processed_cache_key}:#{second_note.post_processed_cache_key}:#{third_note.post_processed_cache_key}") }
it 'returns the cache key with ID and latest updated note updated at' do
expect(subject.cache_key).to eq("#{described_class::CACHE_VERSION}:#{third_note.latest_cached_markdown_version}:#{subject.id}:#{notes_sha}:#{third_note.updated_at}:")
it 'returns the cache key' do
expect(subject.cache_key).to eq("#{described_class::CACHE_VERSION}:#{subject.id}:#{notes_sha}:")
end
context 'when discussion is resolved' do
......@@ -65,7 +65,7 @@ RSpec.describe Discussion do
end
it 'returns the cache key with resolved at' do
expect(subject.cache_key).to eq("#{described_class::CACHE_VERSION}:#{third_note.latest_cached_markdown_version}:#{subject.id}:#{notes_sha}:#{third_note.updated_at}:#{subject.resolved_at}")
expect(subject.cache_key).to eq("#{described_class::CACHE_VERSION}:#{subject.id}:#{notes_sha}:#{subject.resolved_at}")
end
end
end
......
......@@ -1538,4 +1538,24 @@ RSpec.describe Note do
expect(attachment).not_to be_exist
end
end
describe '#post_processed_cache_key' do
let(:note) { build(:note) }
it 'returns cache key by default' do
expect(note.post_processed_cache_key).to eq(note.cache_key)
end
context 'when note has redacted_note_html' do
let(:redacted_note_html) { 'redacted note html' }
before do
note.redacted_note_html = redacted_note_html
end
it 'returns cache key with redacted_note_html sha' do
expect(note.post_processed_cache_key).to eq("#{note.cache_key}:#{Digest::SHA1.hexdigest(redacted_note_html)}")
end
end
end
end
......@@ -54,7 +54,8 @@ RSpec.describe 'merge requests discussions' do
end
context 'caching', :use_clean_rails_memory_store_caching do
let!(:first_note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project) }
let(:reference) { create(:issue, project: project) }
let!(:first_note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project, note: "reference: #{reference.to_reference}") }
let!(:second_note) { create(:diff_note_on_merge_request, in_reply_to: first_note, noteable: merge_request, project: project) }
let!(:award_emoji) { create(:award_emoji, awardable: first_note) }
......@@ -93,6 +94,16 @@ RSpec.describe 'merge requests discussions' do
end
end
context 'when a note in a discussion got its reference state updated' do
before do
reference.close!
end
it_behaves_like 'cache miss' do
let(:changed_notes) { [first_note, second_note] }
end
end
context 'when a note in a discussion got resolved' do
before do
travel_to(1.minute.from_now) do
......@@ -147,17 +158,6 @@ RSpec.describe 'merge requests discussions' do
end
end
context 'when cached markdown version gets bump' do
before do
settings = Gitlab::CurrentSettings.current_application_settings
settings.update!(local_markdown_version: settings.local_markdown_version + 1)
end
it_behaves_like 'cache miss' do
let(:changed_notes) { [first_note, second_note] }
end
end
context 'when the diff note position changes' do
before do
# This replicates a position change wherein timestamps aren't updated
......
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