Skip to content

Commit 8640a9f

Browse files
justin808claude
andcommitted
Address current-head review findings on PR #3964
- Require cache/tag_index from cache.rb so direct requires of the cache file keep working (Codex) - Blank tags in revalidate_tags are a no-op returning 0, mirroring register_tags (CodeRabbit) - Validate cache_tag_index_expires_in / cache_tag_index_max_keys with dedicated setters per the neighboring renderer-setting pattern (CodeRabbit) - Expose revalidates_react_cache and the private callback in the Revalidates RBS surface (CodeRabbit + Claude review) - Guard the private Store key-normalization API with a respond_to? check, one-time warning, and raw-key fallback for custom stores (Claude review) - Blank-tag error message now shows both the original entry and the resolved value so Proc-derived blanks are debuggable (Claude review) - Specs: legacy bare-array index payload, blank revalidate no-op, private-API fallback warning, config setter validation Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 7d72ec8 commit 8640a9f

6 files changed

Lines changed: 123 additions & 8 deletions

File tree

react_on_rails_pro/lib/react_on_rails_pro/cache.rb

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
# https://github.com/shakacode/react_on_rails/blob/main/REACT-ON-RAILS-PRO-LICENSE.md
1515

1616
require "react_on_rails/utils"
17+
require "react_on_rails_pro/cache/tag_index"
1718

1819
module ReactOnRailsPro
1920
class Cache
@@ -55,10 +56,14 @@ def register_tags(tags, cache_key, cache_options)
5556

5657
# Deletes every cached component entry registered under the given tags
5758
# and clears the tag index entries. Tags accept the same forms as the
58-
# `cache_tags:` helper option. Missing/evicted index entries are a no-op.
59-
# Returns the number of cache entries deleted.
59+
# `cache_tags:` helper option. Blank tags (nil/empty/whitespace) are
60+
# ignored, mirroring register_tags; missing/evicted index entries are a
61+
# no-op. Returns the number of cache entries deleted.
6062
def revalidate_tags(*tags)
61-
TagIndex.revalidate(*tags)
63+
meaningful_tags = tags.flatten.reject(&:blank?)
64+
return 0 if meaningful_tags.empty?
65+
66+
TagIndex.revalidate(*meaningful_tags)
6267
end
6368

6469
def use_cache?(options)

react_on_rails_pro/lib/react_on_rails_pro/cache/tag_index.rb

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,11 @@ def normalize_tag(tag)
7676
return normalize_tags(resolved) if resolved.is_a?(Array)
7777

7878
value = tag_value(resolved)
79-
raise ReactOnRailsPro::Error, "cache_tags value #{tag.inspect} normalized to a blank tag" if value.blank?
79+
if value.blank?
80+
raise ReactOnRailsPro::Error,
81+
"cache_tags entry resolved to a blank tag " \
82+
"(original: #{tag.inspect}, resolved: #{resolved.inspect})"
83+
end
8084

8185
[value]
8286
end
@@ -212,6 +216,11 @@ def delete_entries(keys)
212216
end
213217
end
214218

219+
# The private Store methods normalized_entry_key reproduces. Custom or
220+
# future stores missing any of these fall back to the raw cache key
221+
# (with a one-time warning) instead of failing registration silently.
222+
PRIVATE_KEY_METHODS = %i[expanded_key namespace_key merged_options].freeze
223+
215224
def normalized_entry_key(cache_key, cache_options)
216225
# Record the store's *logical* cache name: the expanded key plus any
217226
# :namespace from the entry's cache_options or the store default —
@@ -226,8 +235,26 @@ def normalized_entry_key(cache_key, cache_options)
226235
# these private Store methods is the only way to reproduce the
227236
# store's naming; all three have been stable across ActiveSupport
228237
# versions for years.
229-
expanded = Rails.cache.send(:expanded_key, cache_key)
230-
Rails.cache.send(:namespace_key, expanded, Rails.cache.send(:merged_options, cache_options))
238+
store = Rails.cache
239+
unless PRIVATE_KEY_METHODS.all? { |method_name| store.respond_to?(method_name, true) }
240+
warn_missing_private_key_api(store)
241+
return cache_key.to_s
242+
end
243+
244+
expanded = store.send(:expanded_key, cache_key)
245+
store.send(:namespace_key, expanded, store.send(:merged_options, cache_options))
246+
end
247+
248+
def warn_missing_private_key_api(store)
249+
@warned_private_key_api ||= {}
250+
return if @warned_private_key_api[store.class]
251+
252+
@warned_private_key_api[store.class] = true
253+
Rails.logger.warn do
254+
"[ReactOnRailsPro] #{store.class} does not implement the private key-normalization API " \
255+
"(#{PRIVATE_KEY_METHODS.join(', ')}); the cache tag index falls back to raw cache keys, " \
256+
"so tag revalidation may miss entries written with a :namespace or expanded key forms."
257+
end
231258
end
232259
end
233260
end

react_on_rails_pro/lib/react_on_rails_pro/configuration.rb

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -115,10 +115,34 @@ class Configuration # rubocop:disable Metrics/ClassLength
115115
:renderer_request_retry_limit, :throw_js_errors, :ssr_timeout,
116116
:profile_server_rendering_js_code, :raise_non_shell_server_rendering_errors, :enable_rsc_support,
117117
:rsc_payload_generation_url_path, :rsc_bundle_js_file, :react_client_manifest_file,
118-
:react_server_client_manifest_file, :cache_tag_index_expires_in, :cache_tag_index_max_keys
118+
:react_server_client_manifest_file
119119

120120
attr_reader :concurrent_component_streaming_buffer_size, :renderer_http_keep_alive_timeout,
121-
:renderer_http_pool_size
121+
:renderer_http_pool_size, :cache_tag_index_expires_in, :cache_tag_index_max_keys
122+
123+
# Sets how long tag->key index entries live (see Cache::TagIndex).
124+
#
125+
# @param value [Numeric] A positive number of seconds (e.g. 7.days)
126+
# @raise [ReactOnRailsPro::Error] if value is not a positive, finite number
127+
def cache_tag_index_expires_in=(value)
128+
unless value.is_a?(Numeric) && value.positive? && value.to_f.finite?
129+
raise ReactOnRailsPro::Error,
130+
"config.cache_tag_index_expires_in must be a positive number of seconds"
131+
end
132+
@cache_tag_index_expires_in = value
133+
end
134+
135+
# Sets the maximum cache-entry keys recorded per tag (see Cache::TagIndex).
136+
#
137+
# @param value [Integer] A positive integer
138+
# @raise [ReactOnRailsPro::Error] if value is not a positive integer
139+
def cache_tag_index_max_keys=(value)
140+
unless value.is_a?(Integer) && value.positive?
141+
raise ReactOnRailsPro::Error,
142+
"config.cache_tag_index_max_keys must be a positive integer"
143+
end
144+
@cache_tag_index_max_keys = value
145+
end
122146

123147
# Sets the buffer size for concurrent component streaming.
124148
#

react_on_rails_pro/sig/react_on_rails_pro/cache.rbs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,14 @@ module ReactOnRailsPro
4242

4343
module Revalidates
4444
def self.included: (Module base) -> void
45+
46+
module ClassMethods
47+
def revalidates_react_cache: () ?{ (untyped record) -> untyped } -> void
48+
end
49+
50+
private
51+
52+
def revalidate_react_on_rails_cache_tags: () -> void
4553
end
4654
end
4755
end

react_on_rails_pro/spec/react_on_rails_pro/cache/tag_index_spec.rb

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -290,5 +290,30 @@ def index_payload(tag)
290290
expect { ReactOnRailsPro::Cache.register_tags(nil, "entry/one", nil) }.not_to raise_error
291291
expect { ReactOnRailsPro::Cache.register_tags([], "entry/one", nil) }.not_to raise_error
292292
end
293+
294+
it "treats blank tags in revalidate_tags as a no-op returning 0" do
295+
expect(ReactOnRailsPro.revalidate_tag(nil)).to eq(0)
296+
expect(ReactOnRailsPro.revalidate_tags(nil, "", " ", [])).to eq(0)
297+
end
298+
299+
it "tolerates a legacy bare-array index payload (no expires_at)" do
300+
Rails.cache.write(described_class.index_key("legacy-tag"), %w[entry/one entry/two])
301+
Rails.cache.write("entry/one", "one")
302+
303+
expect(described_class.revalidate("legacy-tag")).to eq(1)
304+
expect(Rails.cache.read("entry/one")).to be_nil
305+
end
306+
307+
it "falls back to the raw cache key with a warning when the store lacks the private key API" do
308+
bare_store = ActiveSupport::Cache::MemoryStore.new
309+
allow(bare_store).to receive(:respond_to?).and_call_original
310+
allow(bare_store).to receive(:respond_to?).with(:expanded_key, true).and_return(false)
311+
allow(Rails).to receive(:cache).and_return(bare_store)
312+
allow(Rails.logger).to receive(:warn)
313+
described_class.instance_variable_set(:@warned_private_key_api, nil)
314+
315+
expect(described_class.send(:normalized_entry_key, "entry/raw", {})).to eq("entry/raw")
316+
expect(Rails.logger).to have_received(:warn)
317+
end
293318
end
294319
end

react_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rb

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -989,6 +989,32 @@ def self.upload(_hash, _options = {}, **nil); end
989989
end
990990
end
991991

992+
describe ".cache_tag_index_expires_in / .cache_tag_index_max_keys" do
993+
it "accepts valid values" do
994+
ReactOnRailsPro.configure do |config|
995+
config.cache_tag_index_expires_in = 3600
996+
config.cache_tag_index_max_keys = 100
997+
end
998+
999+
expect(ReactOnRailsPro.configuration.cache_tag_index_expires_in).to eq(3600)
1000+
expect(ReactOnRailsPro.configuration.cache_tag_index_max_keys).to eq(100)
1001+
end
1002+
1003+
it "raises on a non-positive or non-numeric expires_in" do
1004+
expect { ReactOnRailsPro.configure { |config| config.cache_tag_index_expires_in = 0 } }
1005+
.to raise_error(ReactOnRailsPro::Error, /cache_tag_index_expires_in/)
1006+
expect { ReactOnRailsPro.configure { |config| config.cache_tag_index_expires_in = "1 day" } }
1007+
.to raise_error(ReactOnRailsPro::Error, /cache_tag_index_expires_in/)
1008+
end
1009+
1010+
it "raises on a non-positive or non-integer max_keys" do
1011+
expect { ReactOnRailsPro.configure { |config| config.cache_tag_index_max_keys = -1 } }
1012+
.to raise_error(ReactOnRailsPro::Error, /cache_tag_index_max_keys/)
1013+
expect { ReactOnRailsPro.configure { |config| config.cache_tag_index_max_keys = 5.5 } }
1014+
.to raise_error(ReactOnRailsPro::Error, /cache_tag_index_max_keys/)
1015+
end
1016+
end
1017+
9921018
describe ".concurrent_component_streaming_buffer_size" do
9931019
it "accepts positive integers" do
9941020
ReactOnRailsPro.configure do |config|

0 commit comments

Comments
 (0)