Skip to content

Cache key_value? lookups with pre-built flat Set per locale #709

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hoblin wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Cache key_value? lookups with pre-built flat Set per locale #709

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions lib/i18n/tasks/data.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,14 @@ def data
adapter_class = adapter_class.to_s
adapter_class = "I18n::Tasks::Data::FileSystem" if adapter_class == "file_system"
data_config.except!(:adapter, :class)
ActiveSupport::Inflector.constantize(adapter_class).new data_config
adapter = ActiveSupport::Inflector.constantize(adapter_class).new data_config
# Wrap reload to invalidate key_value? cache
task = self
adapter.define_singleton_method(:reload) do
task.invalidate_key_value_cache!
super()
Comment on lines +25 to +27
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrapping adapter.reload via define_singleton_method and calling super() assumes every configured data adapter implements reload with a compatible signature. Since data.adapter is configurable, this can raise NoMethodError (no reload) or break if an adapter defines reload(*args) later. Consider guarding with respond_to?(:reload) and forwarding args/kwargs/block (super(*args, **kwargs, &block)), or wrapping via Module#prepend to avoid signature mismatches.

Suggested change
adapter.define_singleton_method(:reload) do
task.invalidate_key_value_cache!
super()
if adapter.respond_to?(:reload)
adapter.define_singleton_method(:reload) do |*args, **kwargs, &block|
task.invalidate_key_value_cache!
super(*args, **kwargs, &block)
end

Copilot uses AI. Check for mistakes.
end
adapter
end
end

Expand Down Expand Up @@ -56,13 +63,19 @@ def t_proc(locale = base_locale)

# whether the value for key exists in locale (defaults: base_locale)
def key_value?(key, locale = base_locale)
!t(key, locale).nil?
@key_value_cache ||= {}
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cache is keyed by locale as passed in. Elsewhere the data layer normalizes locales to strings (FileSystemBase#get(locale) calls locale.to_s), so calling key_value? with both :en and "en" will build duplicate caches and waste memory. Consider normalizing locale = locale.to_s before indexing @key_value_cache and before calling build_key_value_set.

Suggested change
@key_value_cache ||= {}
@key_value_cache ||= {}
locale = locale.to_s

Copilot uses AI. Check for mistakes.
locale_cache = (@key_value_cache[locale] ||= build_key_value_set(locale))
locale_cache.include?(key)
Comment on lines 65 to +68
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

key_value? previously accepted Symbol keys (they were coerced via Siblings#get(full_key.to_s)), but locale_cache.include?(key) will not match because the Set is populated with Strings. To preserve existing behavior, coerce key to a String before lookup (and ideally do the same normalization when building the set).

Copilot uses AI. Check for mistakes.
end

def external_key?(key, locale = base_locale)
data.external(locale)[locale.to_s][key]
end

def invalidate_key_value_cache!
@key_value_cache = nil
end

# Normalize all the locale data in the store (by writing to the store).
#
# @param [Array<String>] locales locales to normalize. Default: all.
Expand All @@ -83,5 +96,13 @@ def normalize_store!(locales: nil, force_pattern_router: false)
def non_normalized_paths(locales: nil)
Array(locales || self.locales).flat_map { |locale| data.non_normalized_paths(locale) }
end

private

def build_key_value_set(locale)
keys = Set.new
data[locale].nodes { |node| keys << node.full_key(root: false) unless node.key.nil? }
keys
Comment on lines +102 to +105
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

key_value? now checks membership in a Set of node keys, which changes semantics for leaf nodes with value == nil. Previously key_value? returned false in that case because t(key, locale) returns nil when the stored translation value is nil (see Node#value_or_children_hash). With the current implementation, those keys will be treated as present, which can hide missing translations. Consider only adding keys for non-leaf nodes, or for leaf nodes where node.value is not nil (i.e., mimic t(...).nil? behavior).

Copilot uses AI. Check for mistakes.
end
end
end
Loading