From 6b0f99f1995f9b0de54b80dbdb91b1b4a5f7dd92 Mon Sep 17 00:00:00 2001 From: Chris Andreae Date: Sat, 11 May 2024 13:20:39 +0900 Subject: [PATCH 1/2] VM::Cache: retry without using cache on stale cached references If a cached value contains a reference to a entity that no longer exists, this currently results in a RecordNotFound error. Instead, we should be able to treat this as a signal that the cached value is out of date, and re-resolve it from the database. --- lib/view_model/active_record/cache.rb | 44 ++++++++++++++++--- .../view_model/active_record/cache_test.rb | 18 +++++++- 2 files changed, 54 insertions(+), 8 deletions(-) diff --git a/lib/view_model/active_record/cache.rb b/lib/view_model/active_record/cache.rb index b3be7b8d..cc61a425 100644 --- a/lib/view_model/active_record/cache.rb +++ b/lib/view_model/active_record/cache.rb @@ -30,8 +30,21 @@ def render_viewmodels_from_cache(viewmodels, migration_versions: {}, locked: fal end def render_from_cache(viewmodel_class, ids, initial_viewmodels: nil, migration_versions: {}, locked: false, serialize_context: viewmodel_class.new_serialize_context) - worker = CacheWorker.new(migration_versions: migration_versions, serialize_context: serialize_context) - worker.render_from_cache(viewmodel_class, ids, initial_viewmodels: initial_viewmodels, locked: locked) + ignore_existing = false + begin + worker = CacheWorker.new(migration_versions: migration_versions, serialize_context: serialize_context, ignore_existing: ignore_existing) + worker.render_from_cache(viewmodel_class, ids, initial_viewmodels: initial_viewmodels, locked: locked) + rescue StaleCachedReference + # If the cache contents contained a unresolvable stale reference, retry + # while ignoring existing cached values (thereby updating the cache). This + # will have the side-effect of duplicate Before/AfterVisit callbacks. + if ignore_existing + raise + else + ignore_existing = true + retry + end + end end end @@ -76,18 +89,25 @@ def fetch(ids, initial_viewmodels: nil, migration_versions: {}, locked: false, s migration_versions: migration_versions, serialize_context: serialize_context) end + class StaleCachedReference < StandardError + def initialize(error) + super("Cached value contained stale reference: #{error.message}") + end + end + class CacheWorker SENTINEL = Object.new WorklistEntry = Struct.new(:ref_name, :viewmodel) attr_reader :migration_versions, :serialize_context, :resolved_references - def initialize(migration_versions:, serialize_context:) + def initialize(migration_versions:, serialize_context:, ignore_existing: false) @worklist = {} # Hash[type_name, Hash[id, WorklistEntry]] @resolved_references = {} # Hash[refname, json] @migration_versions = migration_versions @migrated_cache_versions = {} @serialize_context = serialize_context + @ignore_existing = ignore_existing end def render_from_cache(viewmodel_class, ids, initial_viewmodels: nil, locked: false) @@ -104,7 +124,7 @@ def render_from_cache(viewmodel_class, ids, initial_viewmodels: nil, locked: fal ids_to_render = ids.to_set - if viewmodel_class < CacheableView + if viewmodel_class < CacheableView && !@ignore_existing # Load existing serializations from the cache cached_serializations = load_from_cache(viewmodel_class.viewmodel_cache, ids) cached_serializations.each do |id, data| @@ -168,7 +188,7 @@ def resolve_references! @resolved_references[entry.ref_name] = SENTINEL end - if viewmodel_class < CacheableView + if viewmodel_class < CacheableView && !@ignore_existing cached_serializations = load_from_cache(viewmodel_class.viewmodel_cache, required_entries.keys) cached_serializations.each do |id, data| ref_name = required_entries.delete(id).ref_name @@ -181,8 +201,18 @@ def resolve_references! h[id] = entry.viewmodel if entry.viewmodel end - viewmodels = find_and_preload_viewmodels(viewmodel_class, required_entries.keys, - available_viewmodels: available_viewmodels) + viewmodels = + begin + find_and_preload_viewmodels(viewmodel_class, required_entries.keys, + available_viewmodels: available_viewmodels) + rescue ViewModel::DeserializationError::NotFound => e + # We encountered a reference to an entity that does not exist. + # If this reference was potentially found in cached data, it + # could be stale: we can retry without using the cache. + # If the reference was obtained directly, it indicates invalid + # data such as an invalid foreign key, and we cannot recover. + raise StaleCachedReference.new(e) + end loaded_serializations = serialize_and_cache(viewmodels) loaded_serializations.each do |id, data| diff --git a/test/unit/view_model/active_record/cache_test.rb b/test/unit/view_model/active_record/cache_test.rb index 5fb4dcc0..7bf4e5a6 100644 --- a/test/unit/view_model/active_record/cache_test.rb +++ b/test/unit/view_model/active_record/cache_test.rb @@ -209,11 +209,27 @@ module BehavesLikeACache end it 'returns the right serialization with provided locked initial viewmodel' do - locked_root_view = viewmodel_class.new(model_class.lock("FOR SHARE").find(root.id)) + locked_root_view = viewmodel_class.new(model_class.lock('FOR SHARE').find(root.id)) fetched_result = parse_result(fetch_with_cache(initial_viewmodels: [locked_root_view], locked: true)) value(fetched_result).must_equal(serialize_from_database) end + + it 'handles a stale cached reference' do + # Fetch to populate the cache + initial_result = parse_result(fetch_with_cache) + value(initial_result).must_equal(serialize_from_database) + + # Destroy the shared child and its cache without invalidating the cached parent cache + root.update_columns(shared_id: nil) + shared.destroy! + shared_viewmodel_class.viewmodel_cache.clear + + fetched_result = parse_result(fetch_with_cache) + value(fetched_result).must_equal(serialize_from_database) + fetched_view = fetched_result.first.first + value(fetched_view['shared']).must_be_nil + end end describe 'with migrations' do From 7b596f939b8283e029bebd176896054f1b1c810c Mon Sep 17 00:00:00 2001 From: Chris Andreae Date: Thu, 23 May 2024 17:19:55 +0900 Subject: [PATCH 2/2] Update gem version --- lib/iknow_view_models/version.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/iknow_view_models/version.rb b/lib/iknow_view_models/version.rb index 8f80ae3b..2186409c 100644 --- a/lib/iknow_view_models/version.rb +++ b/lib/iknow_view_models/version.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true module IknowViewModels - VERSION = '3.10.0' + VERSION = '3.10.1' end