From 1c1c2010fb433b26dc978faeb140cd53faa05887 Mon Sep 17 00:00:00 2001 From: Erik Axel Nielsen Date: Thu, 3 Sep 2026 13:55:09 +0200 Subject: [PATCH] Report the errors swallowed while computing a cache digest Every rescue in the digest machinery returns a neutral value, so a misconfiguration, an autoload failure, or a raising `inherited` hook degrades to "no component dependencies" and the application serves stale HTML with nothing reported anywhere. - Route the four swallow sites through CacheDigest.handle_error - Log at `warn` through ActiveSupport's logger, preserving the production guarantee that a stale fragment beats a failed render - Raise instead in local environments, configurable with config.view_component.raise_on_cache_digest_errors --- docs/CHANGELOG.md | 4 + docs/api.md | 13 ++ docs/guide/caching.md | 16 +++ lib/view_component/cache_digest.rb | 33 ++++- .../cache_digest/dependency_tracking.rb | 8 +- lib/view_component/cache_digest/resolver.rb | 9 +- lib/view_component/config.rb | 21 +++- lib/view_component/engine.rb | 1 + test/sandbox/test/config_test.rb | 1 + .../test/experimentally_cacheable_test.rb | 113 ++++++++++++++++-- 10 files changed, 193 insertions(+), 26 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 9834fc583..7e57c5e9c 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -10,6 +10,10 @@ nav_order: 6 ## main +* Report the errors swallowed while computing a component's cache digest, instead of degrading to an untracked component with no indication that anything went wrong. Digest errors are now raised in local environments and logged at `warn` elsewhere, configurable with `config.view_component.raise_on_cache_digest_errors`. + + *Erik Axel Nielsen* + ## 4.15.0 * Add experimental caching support, opt-in per component via `include ViewComponent::ExperimentallyCacheable`. diff --git a/docs/api.md b/docs/api.md index b615b2d54..859b75cf1 100644 --- a/docs/api.md +++ b/docs/api.md @@ -294,6 +294,19 @@ A custom default layout used for the previews index page and individual previews config.view_component.previews.default_layout = "preview_layout" +### `.raise_on_cache_digest_errors` + +Whether to raise when computing a component's cache digest fails. + +Digest failures are otherwise swallowed, since a stale fragment is +preferable to a failed render, and reported to the log at `warn`. That +trade is wrong in development and test, where an untracked component +looks exactly like a component that was never cached. + +Defaults to `true` in local environments and `false` elsewhere: + + config.view_component.raise_on_cache_digest_errors = false + ## ViewComponent::TestHelpers ### `#render_in_view_context(...)` diff --git a/docs/guide/caching.md b/docs/guide/caching.md index 183de010d..cc46681f1 100644 --- a/docs/guide/caching.md +++ b/docs/guide/caching.md @@ -189,6 +189,22 @@ The same works in a template, where the branch is often the more natural place f Declared components must include `ViewComponent::ExperimentallyCacheable` themselves, since a component that hasn't opted in has no digest to depend on. +## When a digest can't be computed + +Computing a digest touches the autoloader, the filesystem, and Action View's dependency trackers, any of which can fail. In production those failures are swallowed: a component that can't be digested is left untracked, which is exactly the behavior it had before opting in, and a stale fragment beats a failed render. + +That trade is wrong while developing, where an untracked component is indistinguishable from a component that was never cached in the first place. Digest failures are therefore raised in local environments and logged at `warn` everywhere else: + +```console +[ViewComponent] Ignored an error while resolving PostComponent: NoMethodError: ... +``` + +To swallow them locally too, or to raise them in production: + +```ruby +config.view_component.raise_on_cache_digest_errors = false +``` + ## Caveats **Self-caching components can't take content from their callers.** Besides a block, this covers `with_content` and slots set by the caller: diff --git a/lib/view_component/cache_digest.rb b/lib/view_component/cache_digest.rb index 98694c8dc..cb58eaf0c 100644 --- a/lib/view_component/cache_digest.rb +++ b/lib/view_component/cache_digest.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require "active_support/dependencies/autoload" +require "active_support/log_subscriber" require "action_view/digestor" require "action_view/render_parser" @@ -146,8 +147,8 @@ def partial_paths_in(source, name) RENDER_PARSER.new(name, source).render_calls.uniq.select do |path| source.include?(path) || source.include?(path.sub(%r{(\A|/)_}, '\1')) end - rescue - # Never let digest computation break rendering. + rescue => error + handle_error(error, "scanning #{name} for rendered partials") [] end @@ -221,6 +222,30 @@ def install! end end + # Report an exception the digest machinery swallowed. + # + # Every rescue here degrades to "this component has no dependencies", + # which is indistinguishable from a component that was never cached: the + # digest stops changing and the application serves stale HTML. A + # misconfiguration, an autoload failure, or a raising `inherited` hook is + # therefore completely invisible. + # + # In production a stale fragment beats a failed render, so the exception + # is only logged. Locally the trade goes the other way, so it's re-raised + # by default; see `config.view_component.raise_on_cache_digest_errors`. + # + # @private + def handle_error(error, context) + raise error if ViewComponent::Base.config.raise_on_cache_digest_errors + + logger&.warn { "[ViewComponent] Ignored an error while #{context}: #{error.class}: #{error.message}" } + end + + # @return [ActiveSupport::BroadcastLogger, Logger, nil] nil outside a Rails application. + def logger + ActiveSupport::LogSubscriber.logger + end + private # Resolve a constant name to a component that opted into caching. @@ -234,8 +259,8 @@ def constantize_component(constant_name) return unless component.respond_to?(:__vc_cacheable?) && component.__vc_cacheable? component - rescue - # Never let digest computation break rendering. + rescue => error + handle_error(error, "resolving #{constant_name}") nil end end diff --git a/lib/view_component/cache_digest/dependency_tracking.rb b/lib/view_component/cache_digest/dependency_tracking.rb index 737616dde..851efc8b2 100644 --- a/lib/view_component/cache_digest/dependency_tracking.rb +++ b/lib/view_component/cache_digest/dependency_tracking.rb @@ -27,10 +27,10 @@ def find_dependencies(name, template, view_paths = nil) end dependencies + CacheDigest.dependencies_in(template) - rescue - # A broken digest is preferable to a broken render. Falling back to the - # dependencies Rails found on its own means the component simply isn't - # tracked, which is the pre-existing behavior. + rescue => error + # Falling back to the dependencies Rails found on its own means the + # component simply isn't tracked, which is the pre-existing behavior. + CacheDigest.handle_error(error, "tracking component dependencies in #{name}") super end diff --git a/lib/view_component/cache_digest/resolver.rb b/lib/view_component/cache_digest/resolver.rb index eca6198bf..9e078aedb 100644 --- a/lib/view_component/cache_digest/resolver.rb +++ b/lib/view_component/cache_digest/resolver.rb @@ -30,10 +30,11 @@ def find_templates(name, prefix, partial, details, locals = []) return [] unless component [build_template(component, virtual_path, details)] - rescue - # Never let digest resolution break rendering. Returning no template - # makes the Digestor treat this as a missing node, which degrades to - # the behavior components have without this feature. + rescue => error + # Returning no template makes the Digestor treat this as a missing + # node, which degrades to the behavior components have without this + # feature. + CacheDigest.handle_error(error, "building the digest template for #{virtual_path || name}") [] end diff --git a/lib/view_component/config.rb b/lib/view_component/config.rb index 0ad336838..7b6bd453b 100644 --- a/lib/view_component/config.rb +++ b/lib/view_component/config.rb @@ -14,7 +14,8 @@ def defaults ActiveSupport::OrderedOptions.new.merge!({ generate: default_generate_options, previews: default_previews_options, - instrumentation_enabled: false + instrumentation_enabled: false, + raise_on_cache_digest_errors: default_raise_on_cache_digest_errors }) end @@ -131,6 +132,20 @@ def defaults # Whether ActiveSupport notifications are enabled. # Defaults to `false`. + # @!attribute raise_on_cache_digest_errors + # + # @return [Boolean] + # Whether to raise when computing a component's cache digest fails. + # + # Digest failures are otherwise swallowed, since a stale fragment is + # preferable to a failed render, and reported to the log at `warn`. That + # trade is wrong in development and test, where an untracked component + # looks exactly like a component that was never cached. + # + # Defaults to `true` in local environments and `false` elsewhere: + # + # config.view_component.raise_on_cache_digest_errors = false + def default_preview_paths (default_rails_preview_paths + default_rails_engines_preview_paths).uniq end @@ -155,6 +170,10 @@ def registered_rails_engines_with_previews end end + def default_raise_on_cache_digest_errors + defined?(Rails.env) && Rails.env.local? + end + def default_generate_options options = ActiveSupport::OrderedOptions.new(false) options.preview_path = "" diff --git a/lib/view_component/engine.rb b/lib/view_component/engine.rb index dc0a6e1c7..be33d4729 100644 --- a/lib/view_component/engine.rb +++ b/lib/view_component/engine.rb @@ -15,6 +15,7 @@ class Engine < Rails::Engine # :nodoc: options[config_option] ||= ViewComponent::Base.public_send(config_option) end options.instrumentation_enabled = false if options.instrumentation_enabled.nil? + options.raise_on_cache_digest_errors = Rails.env.local? if options.raise_on_cache_digest_errors.nil? options.previews.enabled = (Rails.env.development? || Rails.env.test?) if options.previews.enabled.nil? if options.previews.enabled diff --git a/test/sandbox/test/config_test.rb b/test/sandbox/test/config_test.rb index 9347a6ac1..d4380391e 100644 --- a/test/sandbox/test/config_test.rb +++ b/test/sandbox/test/config_test.rb @@ -13,6 +13,7 @@ def test_defaults_are_correct assert_equal @config.previews.controller, "ViewComponentsController" assert_equal @config.previews.route, "/rails/view_components" assert_equal @config.instrumentation_enabled, false + assert_equal @config.raise_on_cache_digest_errors, Rails.env.local? assert_equal @config.previews.paths, ["#{Rails.root}/test/components/previews"] end diff --git a/test/sandbox/test/experimentally_cacheable_test.rb b/test/sandbox/test/experimentally_cacheable_test.rb index bb0566163..4e09fae57 100644 --- a/test/sandbox/test/experimentally_cacheable_test.rb +++ b/test/sandbox/test/experimentally_cacheable_test.rb @@ -204,8 +204,22 @@ def test_partial_paths_are_not_extracted_from_sources_without_render end def test_partial_path_extraction_swallows_parser_errors + swallowing_digest_errors do |log| + ViewComponent::CacheDigest::RENDER_PARSER.stub(:new, ->(*) { raise "boom" }) do + assert_empty ViewComponent::CacheDigest.partial_paths_in("render \"a/b\"", "a/b") + end + + assert_match "Ignored an error while scanning a/b for rendered partials: RuntimeError: boom", log.string + end + end + + def test_partial_path_extraction_raises_parser_errors_locally ViewComponent::CacheDigest::RENDER_PARSER.stub(:new, ->(*) { raise "boom" }) do - assert_empty ViewComponent::CacheDigest.partial_paths_in("render \"a/b\"", "a/b") + error = assert_raises(RuntimeError) do + ViewComponent::CacheDigest.partial_paths_in("render \"a/b\"", "a/b") + end + + assert_equal "boom", error.message end end @@ -437,32 +451,74 @@ def test_resolver_is_identified_by_class def test_resolver_returns_no_template_when_synthesis_fails resolver = ViewComponent::CacheDigest::Resolver.instance + swallowing_digest_errors do |log| + ViewComponent::CacheDigest.stub(:component_for, ->(_) { raise "boom" }) do + assert_empty resolver.find_templates("cacheable_component", "view_component/cache_digest", true, {}) + end + + assert_match( + "Ignored an error while building the digest template for " \ + "view_component/cache_digest/cacheable_component: RuntimeError: boom", + log.string + ) + end + end + + def test_resolver_raises_when_synthesis_fails_locally + resolver = ViewComponent::CacheDigest::Resolver.instance + ViewComponent::CacheDigest.stub(:component_for, ->(_) { raise "boom" }) do - assert_empty resolver.find_templates("cacheable_component", "view_component/cache_digest", true, {}) + assert_raises(RuntimeError) do + resolver.find_templates("cacheable_component", "view_component/cache_digest", true, {}) + end end end def test_dependency_tracking_falls_back_when_scanning_fails template = build_template("<%= render CacheableComponent.new(title: 'a') %>") + swallowing_digest_errors do |log| + ViewComponent::CacheDigest.stub(:dependencies_in, ->(_) { raise "boom" }) do + refute_includes( + ActionView::DependencyTracker.find_dependencies("some/template", template, []), + "view_component/cache_digest/cacheable_component" + ) + end + + assert_match "Ignored an error while tracking component dependencies in some/template", log.string + end + end + + def test_dependency_tracking_raises_when_scanning_fails_locally + template = build_template("<%= render CacheableComponent.new(title: 'a') %>") + ViewComponent::CacheDigest.stub(:dependencies_in, ->(_) { raise "boom" }) do - refute_includes( - ActionView::DependencyTracker.find_dependencies("some/template", template, []), - "view_component/cache_digest/cacheable_component" - ) + assert_raises(RuntimeError) { ActionView::DependencyTracker.find_dependencies("some/template", template, []) } end end def test_constantizing_swallows_unexpected_errors - Object.const_set(:BoomComponent, Class.new do - def self.__vc_cacheable? - raise ArgumentError + with_boom_component do + swallowing_digest_errors do |log| + assert_nil ViewComponent::CacheDigest.send(:constantize_component, "BoomComponent") + + assert_match "Ignored an error while resolving BoomComponent: ArgumentError", log.string end - end) + end + end - assert_nil ViewComponent::CacheDigest.send(:constantize_component, "BoomComponent") - ensure - Object.send(:remove_const, :BoomComponent) + def test_constantizing_raises_unexpected_errors_locally + with_boom_component do + assert_raises(ArgumentError) { ViewComponent::CacheDigest.send(:constantize_component, "BoomComponent") } + end + end + + def test_digest_errors_are_swallowed_without_a_logger + without_raising_digest_errors do + ViewComponent::CacheDigest.stub(:logger, nil) do + assert_nil ViewComponent::CacheDigest.handle_error(RuntimeError.new("boom"), "digesting") + end + end end def test_install_is_idempotent @@ -487,6 +543,37 @@ def recompile(component) component.__vc_compile(force: true) end + # The digest machinery raises in local environments, and the sandbox runs as + # `test`, so the swallow-and-report path has to be opted into explicitly. + def without_raising_digest_errors + previous = ViewComponent::Base.config.raise_on_cache_digest_errors + ViewComponent::Base.config.raise_on_cache_digest_errors = false + + yield + ensure + ViewComponent::Base.config.raise_on_cache_digest_errors = previous + end + + def swallowing_digest_errors + log = StringIO.new + + without_raising_digest_errors do + ViewComponent::CacheDigest.stub(:logger, ActiveSupport::Logger.new(log)) { yield log } + end + end + + def with_boom_component + Object.const_set(:BoomComponent, Class.new do + def self.__vc_cacheable? + raise ArgumentError + end + end) + + yield + ensure + Object.send(:remove_const, :BoomComponent) + end + def build_template(source) ActionView::Template.new( source,