From a9e455f4844923f170915fca21efa60f7223fc1d Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Mon, 24 Feb 2020 13:04:12 -0800 Subject: [PATCH 1/2] Mostly remove bad test This test was attempting to test how cache keys work by modifying the templates and seeing when that cache was fresh. This doesn't actually work for real Resolvers, only FixtureResolver, and isn't desirable. We absolutely want to share templates if they resolve to the same file. Instead, this simplifies the test to only check that we get the correct template for the locale we request. --- .../test/template/lookup_context_test.rb | 29 +++++++++---------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/actionview/test/template/lookup_context_test.rb b/actionview/test/template/lookup_context_test.rb index 72e1f50fdf..f139e2b27e 100644 --- a/actionview/test/template/lookup_context_test.rb +++ b/actionview/test/template/lookup_context_test.rb @@ -190,33 +190,30 @@ class LookupContextTest < ActiveSupport::TestCase assert_equal 3, keys.uniq.size end - test "gives the key forward to the resolver, so it can be used as cache key" do - @lookup_context = build_lookup_context(ActionView::FixtureResolver.new("test/_foo.erb" => "Foo"), {}) + test "uses details as part of cache key" do + fixtures = { + "test/_foo.erb" => "Foo", + "test/_foo.da.erb" => "Bar", + } + @lookup_context = build_lookup_context(ActionView::FixtureResolver.new(fixtures), {}) + template = @lookup_context.find("foo", %w(test), true) + original_template = template assert_equal "Foo", template.source - # Now we are going to change the template, but it won't change the returned template - # since we will hit the cache. - @lookup_context.view_paths.first.data["test/_foo.erb"] = "Bar" + # We should get the same template template = @lookup_context.find("foo", %w(test), true) - assert_equal "Foo", template.source + assert_same original_template, template - # This time we will change the locale. The updated template should be picked since - # lookup_context generated a new key after we changed the locale. + # Using a different locale we get a different view @lookup_context.locale = :da template = @lookup_context.find("foo", %w(test), true) assert_equal "Bar", template.source - # Now we will change back the locale and it will still pick the old template. - # This is expected because lookup_context will reuse the previous key for :en locale. + # Using en we get the original view @lookup_context.locale = :en template = @lookup_context.find("foo", %w(test), true) - assert_equal "Foo", template.source - - # Finally, we can expire the cache. And the expected template will be used. - @lookup_context.view_paths.first.clear_cache - template = @lookup_context.find("foo", %w(test), true) - assert_equal "Bar", template.source + assert_same original_template, template end test "can disable the cache on demand" do From c3c1c32f4b3a2505cf78933251dc94dd9f10168a Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Mon, 24 Feb 2020 11:47:29 -0800 Subject: [PATCH 2/2] Simplify FixtureResolver to reuse filtering logic Previously FixtureResolver had a copy-pasted version of the filtering already done by OptimizedFileSystemResolver. This PR replaces this by extracting the two places actual filesystem operations into separate methods and overriding those. It would be nice to not rely on overriding methods at all, and to extract the actual filtering into a separate, reusable class, but I don't want to do that until some other changes are made to the filtering. This should also make FixtureResolver much more accurate to OptimizedFileSystemResolver, by also creating and caching the UnboundTemplate classes, which de-duplicate templates. --- .../lib/action_view/template/resolver.rb | 18 ++++++++-- .../lib/action_view/testing/resolvers.rb | 35 +++++-------------- 2 files changed, 24 insertions(+), 29 deletions(-) diff --git a/actionview/lib/action_view/template/resolver.rb b/actionview/lib/action_view/template/resolver.rb index 2a5aaba883..cc3d8bb072 100644 --- a/actionview/lib/action_view/template/resolver.rb +++ b/actionview/lib/action_view/template/resolver.rb @@ -204,9 +204,13 @@ module ActionView end end + def source_for_template(template) + Template::Sources::File.new(template) + end + def build_unbound_template(template, virtual_path) handler, format, variant = extract_handler_and_format_and_variant(template) - source = Template::Sources::File.new(template) + source = source_for_template(template) UnboundTemplate.new( source, @@ -316,14 +320,22 @@ module ActionView end private - def find_template_paths_from_details(path, details) + def find_candidate_template_paths(path) # Instead of checking for every possible path, as our other globs would # do, scan the directory for files with the right prefix. query = "#{escape_entry(File.join(@path, path))}*" + Dir[query].reject do |filename| + File.directory?(filename) + end + end + + def find_template_paths_from_details(path, details) + candidates = find_candidate_template_paths(path) + regex = build_regex(path, details) - Dir[query].uniq.reject do |filename| + candidates.uniq.reject do |filename| # This regex match does double duty of finding only files which match # details (instead of just matching the prefix) and also filtering for # case-insensitive file systems. diff --git a/actionview/lib/action_view/testing/resolvers.rb b/actionview/lib/action_view/testing/resolvers.rb index 03eac29bb4..df73d34d77 100644 --- a/actionview/lib/action_view/testing/resolvers.rb +++ b/actionview/lib/action_view/testing/resolvers.rb @@ -27,34 +27,17 @@ module ActionView #:nodoc: end private - def query(path, exts, _, locals, cache:) - regex = build_regex(path, exts) - - @hash.select do |_path, _| - ("/" + _path).match?(regex) - end.map do |_path, source| - handler, format, variant = extract_handler_and_format_and_variant(_path) - - Template.new(source, _path, handler, - virtual_path: path.virtual, - format: format, - variant: variant, - locals: locals - ) - end.sort_by do |t| - match = ("/" + t.identifier).match(regex) - EXTENSIONS.keys.reverse.map do |ext| - if ext == :variants && exts[ext] == :any - match[ext].nil? ? 0 : 1 - elsif match[ext].nil? - exts[ext].length - else - found = match[ext].to_sym - exts[ext].index(found) - end - end + def find_candidate_template_paths(path) + @hash.keys.select do |fixture| + fixture.start_with?(path.virtual) + end.map do |fixture| + "/#{fixture}" end end + + def source_for_template(template) + @hash[template[1..template.size]] + end end class NullResolver < PathResolver