diff --git a/app/commands/concept/cached_content/retrieve.rb b/app/commands/concept/cached_content/retrieve.rb index e187102795..0ee64ef68c 100644 --- a/app/commands/concept/cached_content/retrieve.rb +++ b/app/commands/concept/cached_content/retrieve.rb @@ -25,9 +25,16 @@ def call private def cached = S3Cache::Read.(cache_key) + # The content built here is handed straight to the deferred write rather + # than being regenerated by it, which would parse the documents twice. + # + # The key is captured here too, not recomputed in the job. It is derived + # from synced_to_git_sha, and the job reloads the concept through a + # GlobalID, so a sync landing in between would file this content under + # the new sha's key, where it would be read back as that sync's content. def generate! - Concept::CachedContent::Generate.(concept).tap do - Concept::CachedContent::Store.defer(concept) + Concept::CachedContent::Generate.(concept).tap do |content| + S3Cache::Write.defer(cache_key, content) end end diff --git a/app/commands/concept/cached_content/store.rb b/app/commands/concept/cached_content/store.rb deleted file mode 100644 index fc149b6537..0000000000 --- a/app/commands/concept/cached_content/store.rb +++ /dev/null @@ -1,21 +0,0 @@ -# Generates a concept's parsed content and writes it to the S3 cache. -# Deferred from Retrieve on a cache miss so the write (and the repeat -# generation it needs) happens off the request path. -class Concept::CachedContent::Store - include Mandate - - queue_as :background - - initialize_with :concept - - def call - S3Cache::Write.(cache_key, Concept::CachedContent::Generate.(concept)) - end - - private - # Must match Concept::CachedContent::Retrieve#cache_key - def cache_key - uuid = concept.uuid - "concept-content/#{uuid[0, 2]}/#{uuid[2, 2]}/#{uuid}/#{concept.synced_to_git_sha}.json" - end -end diff --git a/app/commands/exercise/cached_content/retrieve.rb b/app/commands/exercise/cached_content/retrieve.rb index e2cf9585c9..82cabc6503 100644 --- a/app/commands/exercise/cached_content/retrieve.rb +++ b/app/commands/exercise/cached_content/retrieve.rb @@ -27,9 +27,16 @@ def call private def cached = S3Cache::Read.(cache_key) + # The content built here is handed straight to the deferred write rather + # than being regenerated by it, which would parse the documents twice. + # + # The key is captured here too, not recomputed in the job. It is derived + # from the sha we read at, and the job reloads both records through + # GlobalIDs, so a sync landing in between would file this content under + # the new sha's key, where it would be read back as that sync's content. def generate! - Exercise::CachedContent::Generate.(exercise, solution).tap do - Exercise::CachedContent::Store.defer(exercise, solution) + Exercise::CachedContent::Generate.(exercise, solution).tap do |content| + S3Cache::Write.defer(cache_key, content) end end diff --git a/app/commands/exercise/cached_content/store.rb b/app/commands/exercise/cached_content/store.rb deleted file mode 100644 index d3d7923423..0000000000 --- a/app/commands/exercise/cached_content/store.rb +++ /dev/null @@ -1,22 +0,0 @@ -# Generates an exercise's parsed content and writes it to the S3 cache. -# Deferred from Retrieve on a cache miss so the write (and the repeat -# generation it needs) happens off the request path. -class Exercise::CachedContent::Store - include Mandate - - queue_as :background - - initialize_with :exercise, :solution - - def call - S3Cache::Write.(cache_key, Exercise::CachedContent::Generate.(exercise, solution)) - end - - private - # Must match Exercise::CachedContent::Retrieve#cache_key - def cache_key - uuid = exercise.uuid - sha = solution ? solution.git_sha : exercise.git_sha - "exercise-content/#{uuid[0, 2]}/#{uuid[2, 2]}/#{uuid}/#{sha}.json" - end -end diff --git a/app/commands/solution/cached_serialized_view/retrieve.rb b/app/commands/solution/cached_serialized_view/retrieve.rb index 91ed3b0236..d24785e034 100644 --- a/app/commands/solution/cached_serialized_view/retrieve.rb +++ b/app/commands/solution/cached_serialized_view/retrieve.rb @@ -19,9 +19,18 @@ def call private def cached = S3Cache::Read.(cache_key) + # The payload built here is handed straight to the deferred write + # rather than being regenerated by it: a miss otherwise serializes the + # solution twice, and misses are the common case while the cache fills. + # + # The key is captured here too, not recomputed in the job. It is derived + # from updated_at, and the job reloads the solution through a GlobalID — + # so a solution that changes between request and job would file this + # (now old) payload under the new version's key, where it would be read + # back as current. Capturing both together keeps them the same version. def generate! - Solution::CachedSerializedView::Generate.(solution).tap do - Solution::CachedSerializedView::Store.defer(solution) + Solution::CachedSerializedView::Generate.(solution).tap do |payload| + S3Cache::Write.defer(cache_key, payload) end end diff --git a/app/commands/solution/cached_serialized_view/store.rb b/app/commands/solution/cached_serialized_view/store.rb deleted file mode 100644 index 7ed7b5f5ee..0000000000 --- a/app/commands/solution/cached_serialized_view/store.rb +++ /dev/null @@ -1,21 +0,0 @@ -# Generates the solution-view payload and writes it to the S3 cache. -# Deferred from Retrieve on a cache miss so the write (and the repeat -# generation it needs) happens off the request path. -class Solution::CachedSerializedView::Store - include Mandate - - queue_as :background - - initialize_with :solution - - def call - S3Cache::Write.(cache_key, Solution::CachedSerializedView::Generate.(solution)) - end - - private - # Must match Solution::CachedSerializedView::Retrieve#cache_key - def cache_key - uuid = solution.uuid - "solution-view/#{uuid[0, 2]}/#{uuid[2, 2]}/#{uuid}/#{solution.updated_at.to_i}.json" - end -end diff --git a/test/commands/concept/cached_content/retrieve_test.rb b/test/commands/concept/cached_content/retrieve_test.rb index fbe172121e..4f67171f5b 100644 --- a/test/commands/concept/cached_content/retrieve_test.rb +++ b/test/commands/concept/cached_content/retrieve_test.rb @@ -12,7 +12,7 @@ class Concept::CachedContent::RetrieveTest < ActiveSupport::TestCase { about: "

cached about

", introduction: "

cached intro

" }.to_json ) - Concept::CachedContent::Store.expects(:defer).never + S3Cache::Write.expects(:defer).never assert_equal( { about: "

cached about

", introduction: "

cached intro

" }, @@ -23,7 +23,7 @@ class Concept::CachedContent::RetrieveTest < ActiveSupport::TestCase test "cache miss generates live and defers a write" do concept = create :concept - Concept::CachedContent::Store.expects(:defer).with(concept) + S3Cache::Write.expects(:defer).with(cache_key(concept, concept.synced_to_git_sha), anything) assert_equal( Concept::CachedContent::Generate.(concept), @@ -31,6 +31,16 @@ class Concept::CachedContent::RetrieveTest < ActiveSupport::TestCase ) end + test "cache miss parses once, handing the generated content to the write" do + concept = create :concept + generated = { about: "

about

", introduction: "

intro

" } + + Concept::CachedContent::Generate.expects(:call).with(concept).once.returns(generated) + S3Cache::Write.expects(:defer).with(cache_key(concept, concept.synced_to_git_sha), generated) + + assert_equal generated, Concept::CachedContent::Retrieve.(concept) + end + test "an old sha's cache entry is not read after the concept syncs" do concept = create :concept upload_to_s3( @@ -39,7 +49,7 @@ class Concept::CachedContent::RetrieveTest < ActiveSupport::TestCase ) concept.update!(synced_to_git_sha: "new-sha") - Concept::CachedContent::Store.expects(:defer).with(concept) + S3Cache::Write.expects(:defer).with(cache_key(concept, concept.synced_to_git_sha), anything) refute_equal "

stale

", Concept::CachedContent::Retrieve.(concept)[:about] end diff --git a/test/commands/concept/cached_content/store_test.rb b/test/commands/concept/cached_content/store_test.rb deleted file mode 100644 index e4ac743206..0000000000 --- a/test/commands/concept/cached_content/store_test.rb +++ /dev/null @@ -1,19 +0,0 @@ -require 'test_helper' - -class Concept::CachedContent::StoreTest < ActiveSupport::TestCase - setup do - setup_s3_cache_bucket! - end - - test "writes the generated payload to the sharded sha-versioned key" do - concept = create :concept - - Concept::CachedContent::Store.(concept) - - uuid = concept.uuid - key = "concept-content/#{uuid[0, 2]}/#{uuid[2, 2]}/#{uuid}/#{concept.synced_to_git_sha}.json" - - expected = JSON.parse(Concept::CachedContent::Generate.(concept).to_json, symbolize_names: true) - assert_equal expected, S3Cache::Read.(key) - end -end diff --git a/test/commands/exercise/cached_content/retrieve_test.rb b/test/commands/exercise/cached_content/retrieve_test.rb index 899fca337b..9d7c4015a6 100644 --- a/test/commands/exercise/cached_content/retrieve_test.rb +++ b/test/commands/exercise/cached_content/retrieve_test.rb @@ -13,7 +13,7 @@ class Exercise::CachedContent::RetrieveTest < ActiveSupport::TestCase { introduction: "

cached intro

", instructions: "

cached instructions

" }.to_json ) - Exercise::CachedContent::Store.expects(:defer).never + S3Cache::Write.expects(:defer).never assert_equal( { introduction: "

cached intro

", instructions: "

cached instructions

" }, @@ -24,7 +24,7 @@ class Exercise::CachedContent::RetrieveTest < ActiveSupport::TestCase test "cache miss generates live and defers a write" do exercise = create :practice_exercise - Exercise::CachedContent::Store.expects(:defer).with(exercise, nil) + S3Cache::Write.expects(:defer).with(cache_key(exercise, exercise.git_sha), anything) assert_equal( Exercise::CachedContent::Generate.(exercise, nil), @@ -32,6 +32,16 @@ class Exercise::CachedContent::RetrieveTest < ActiveSupport::TestCase ) end + test "cache miss parses once, handing the generated content to the write" do + exercise = create :practice_exercise + generated = { introduction: "

intro

", instructions: "

instructions

" } + + Exercise::CachedContent::Generate.expects(:call).with(exercise, nil).once.returns(generated) + S3Cache::Write.expects(:defer).with(cache_key(exercise, exercise.git_sha), generated) + + assert_equal generated, Exercise::CachedContent::Retrieve.(exercise, nil) + end + test "a solution reads the key for the sha it is pinned to" do exercise = create :practice_exercise solution = create :practice_solution, exercise:, git_sha: OLD_GIT_SHA @@ -40,7 +50,7 @@ class Exercise::CachedContent::RetrieveTest < ActiveSupport::TestCase { introduction: "", instructions: "

pinned

" }.to_json ) - Exercise::CachedContent::Store.expects(:defer).never + S3Cache::Write.expects(:defer).never assert_equal "

pinned

", Exercise::CachedContent::Retrieve.(exercise, solution)[:instructions] end @@ -53,7 +63,7 @@ class Exercise::CachedContent::RetrieveTest < ActiveSupport::TestCase { introduction: "", instructions: "

shared

" }.to_json ) - Exercise::CachedContent::Store.expects(:defer).never + S3Cache::Write.expects(:defer).never assert_equal "

shared

", Exercise::CachedContent::Retrieve.(exercise, solution)[:instructions] end @@ -66,7 +76,7 @@ class Exercise::CachedContent::RetrieveTest < ActiveSupport::TestCase ) exercise.update_columns(git_sha: OLD_GIT_SHA) - Exercise::CachedContent::Store.expects(:defer).with(exercise, nil) + S3Cache::Write.expects(:defer).with(cache_key(exercise, exercise.git_sha), anything) refute_equal "

stale

", Exercise::CachedContent::Retrieve.(exercise, nil)[:instructions] end diff --git a/test/commands/exercise/cached_content/store_test.rb b/test/commands/exercise/cached_content/store_test.rb deleted file mode 100644 index 138104682e..0000000000 --- a/test/commands/exercise/cached_content/store_test.rb +++ /dev/null @@ -1,32 +0,0 @@ -require 'test_helper' - -class Exercise::CachedContent::StoreTest < ActiveSupport::TestCase - setup do - setup_s3_cache_bucket! - end - - test "writes the generated payload to the sharded sha-versioned key" do - exercise = create :practice_exercise - - Exercise::CachedContent::Store.(exercise, nil) - - uuid = exercise.uuid - key = "exercise-content/#{uuid[0, 2]}/#{uuid[2, 2]}/#{uuid}/#{exercise.git_sha}.json" - - expected = JSON.parse(Exercise::CachedContent::Generate.(exercise, nil).to_json, symbolize_names: true) - assert_equal expected, S3Cache::Read.(key) - end - - test "writes to the solution's pinned sha" do - exercise = create :practice_exercise - solution = create :practice_solution, exercise:, git_sha: OLD_GIT_SHA - - Exercise::CachedContent::Store.(exercise, solution) - - uuid = exercise.uuid - assert S3Cache::Read.("exercise-content/#{uuid[0, 2]}/#{uuid[2, 2]}/#{uuid}/#{OLD_GIT_SHA}.json") - end - - # A real commit in the test repo, so content can still be read at it. - OLD_GIT_SHA = '0b04b8976650d993ecf4603cf7413f3c6b898eff'.freeze -end diff --git a/test/commands/solution/cached_serialized_view/retrieve_test.rb b/test/commands/solution/cached_serialized_view/retrieve_test.rb index de65f5841d..b7bc5e6ec0 100644 --- a/test/commands/solution/cached_serialized_view/retrieve_test.rb +++ b/test/commands/solution/cached_serialized_view/retrieve_test.rb @@ -7,11 +7,10 @@ class Solution::CachedSerializedView::RetrieveTest < ActiveSupport::TestCase test "cache hit returns the stored payload without deferring a write" do solution = create(:practice_solution, :published) - uuid = solution.uuid - key = "solution-view/#{uuid[0, 2]}/#{uuid[2, 2]}/#{uuid}/#{solution.updated_at.to_i}.json" - upload_to_s3(Exercism.config.aws_cache_bucket, key, { iterations: [], language: "cached" }.to_json) + upload_to_s3(Exercism.config.aws_cache_bucket, cache_key_for(solution), + { iterations: [], language: "cached" }.to_json) - Solution::CachedSerializedView::Store.expects(:defer).never + S3Cache::Write.expects(:defer).never assert_equal( { iterations: [], language: "cached" }, @@ -23,7 +22,7 @@ class Solution::CachedSerializedView::RetrieveTest < ActiveSupport::TestCase solution = create(:practice_solution, :published) create(:iteration, solution:) - Solution::CachedSerializedView::Store.expects(:defer).with(solution) + S3Cache::Write.expects(:defer).with(cache_key_for(solution), anything) payload = Solution::CachedSerializedView::Retrieve.(solution) @@ -31,14 +30,38 @@ class Solution::CachedSerializedView::RetrieveTest < ActiveSupport::TestCase assert_equal solution.track.highlightjs_language, payload[:language] end + test "cache miss serializes once, handing the generated payload to the write" do + solution = create(:practice_solution, :published) + create(:iteration, solution:) + generated = { iterations: [], language: "generated-once" } + + Solution::CachedSerializedView::Generate.expects(:call).with(solution).once.returns(generated) + S3Cache::Write.expects(:defer).with(cache_key_for(solution), generated) + + assert_equal generated, Solution::CachedSerializedView::Retrieve.(solution) + end + + test "write is keyed on the version that was serialized, not a later one" do + solution = create(:practice_solution, :published) + key_at_read_time = cache_key_for(solution) + + S3Cache::Write.expects(:defer).with(key_at_read_time, anything) + Solution::CachedSerializedView::Retrieve.(solution) + + # Had the key been recomputed when the deferred job ran, a solution + # changing in the meantime would have filed this payload under the new + # version's key, where it would be read back as current. + solution.update!(updated_at: solution.updated_at + 1.hour) + refute_equal key_at_read_time, cache_key_for(solution.reload) + end + test "an old version's cache entry is not read after the solution changes" do solution = create(:practice_solution, :published) - uuid = solution.uuid - old_key = "solution-view/#{uuid[0, 2]}/#{uuid[2, 2]}/#{uuid}/#{solution.updated_at.to_i}.json" - upload_to_s3(Exercism.config.aws_cache_bucket, old_key, { language: "stale" }.to_json) + upload_to_s3(Exercism.config.aws_cache_bucket, cache_key_for(solution), + { language: "stale" }.to_json) solution.update!(updated_at: solution.updated_at + 1.hour) - Solution::CachedSerializedView::Store.expects(:defer).with(solution) + S3Cache::Write.expects(:defer).with(cache_key_for(solution), anything) payload = Solution::CachedSerializedView::Retrieve.(solution) @@ -55,4 +78,10 @@ class Solution::CachedSerializedView::RetrieveTest < ActiveSupport::TestCase Solution::CachedSerializedView::Retrieve.(solution) ) end + + private + def cache_key_for(solution) + uuid = solution.uuid + "solution-view/#{uuid[0, 2]}/#{uuid[2, 2]}/#{uuid}/#{solution.updated_at.to_i}.json" + end end diff --git a/test/commands/solution/cached_serialized_view/store_test.rb b/test/commands/solution/cached_serialized_view/store_test.rb deleted file mode 100644 index b0ab02c9a7..0000000000 --- a/test/commands/solution/cached_serialized_view/store_test.rb +++ /dev/null @@ -1,21 +0,0 @@ -require 'test_helper' - -class Solution::CachedSerializedView::StoreTest < ActiveSupport::TestCase - setup do - setup_s3_cache_bucket! - end - - test "writes the generated payload to the sharded versioned key" do - solution = create(:practice_solution, :published) - create(:iteration, solution:) - - Solution::CachedSerializedView::Store.(solution) - - uuid = solution.uuid - key = "solution-view/#{uuid[0, 2]}/#{uuid[2, 2]}/#{uuid}/#{solution.updated_at.to_i}.json" - stored = S3Cache::Read.(key) - - expected = JSON.parse(Solution::CachedSerializedView::Generate.(solution).to_json, symbolize_names: true) - assert_equal expected, stored - end -end