Merge pull request #35959 from jhawthorn/unbound_templates

De-duplicate templates, introduce UnboundTemplate

7 files changed, 237 insertions, 14 deletions

diff --git a/actionview/lib/action_view.rb b/actionview/lib/action_view.rb
index 5ee14bfc78..7f85bf2a5e 100644
--- a/actionview/lib/action_view.rb
+++ b/actionview/lib/action_view.rb
@@ -44,6 +44,7 @@ module ActionView
     autoload :Rendering
     autoload :RoutingUrlFor
     autoload :Template
+    autoload :UnboundTemplate
     autoload :ViewPaths
 
     autoload_under "renderer" do
diff --git a/actionview/lib/action_view/template/resolver.rb b/actionview/lib/action_view/template/resolver.rb
index e291dc268a..ce53eb046d 100644
--- a/actionview/lib/action_view/template/resolver.rb
+++ b/actionview/lib/action_view/template/resolver.rb
@@ -118,7 +118,7 @@ module ActionView
       locals = locals.map(&:to_s).sort!.freeze
 
       cached(key, [name, prefix, partial], details, locals) do
-        find_templates(name, prefix, partial, details, locals)
+        _find_all(name, prefix, partial, details, key, locals)
       end
     end
 
@@ -131,6 +131,10 @@ module ActionView
 
   private
 
+    def _find_all(name, prefix, partial, details, key, locals)
+      find_templates(name, prefix, partial, details, locals)
+    end
+
     delegate :caching?, to: :class
 
     # This is what child classes implement. No defaults are needed
@@ -169,35 +173,51 @@ module ActionView
       else
         @pattern = DEFAULT_PATTERN
       end
+      @unbound_templates = Concurrent::Map.new
+      super()
+    end
+
+    def clear_cache
+      @unbound_templates.clear
       super()
     end
 
     private
 
-      def find_templates(name, prefix, partial, details, locals)
+      def _find_all(name, prefix, partial, details, key, locals)
         path = Path.build(name, prefix, partial)
-        query(path, details, details[:formats], locals)
+        query(path, details, details[:formats], locals, cache: !!key)
       end
 
-      def query(path, details, formats, locals)
+      def query(path, details, formats, locals, cache:)
         template_paths = find_template_paths_from_details(path, details)
         template_paths = reject_files_external_to_app(template_paths)
 
         template_paths.map do |template|
-          build_template(template, path.virtual, locals)
+          unbound_template =
+            if cache
+              @unbound_templates.compute_if_absent([template, path.virtual]) do
+                build_unbound_template(template, path.virtual)
+              end
+            else
+              build_unbound_template(template, path.virtual)
+            end
+
+          unbound_template.bind_locals(locals)
         end
       end
 
-      def build_template(template, virtual_path, locals)
+      def build_unbound_template(template, virtual_path)
         handler, format, variant = extract_handler_and_format_and_variant(template)
+        source = Template::Sources::File.new(template)
 
-        filename = File.expand_path(template)
-        source = Template::Sources::File.new(filename)
-        Template.new(source, filename, handler,
+        UnboundTemplate.new(
+          source,
+          template,
+          handler,
           virtual_path: virtual_path,
           format: format,
           variant: variant,
-          locals: locals
         )
       end
 
@@ -363,8 +383,8 @@ module ActionView
       [new(""), new("/")]
     end
 
-    def build_template(template, virtual_path, locals)
-      super(template, nil, locals)
+    def build_unbound_template(template, _)
+      super(template, nil)
     end
 
     def reject_files_external_to_app(files)
diff --git a/actionview/lib/action_view/testing/resolvers.rb b/actionview/lib/action_view/testing/resolvers.rb
index a97fb71b26..1bedf44934 100644
--- a/actionview/lib/action_view/testing/resolvers.rb
+++ b/actionview/lib/action_view/testing/resolvers.rb
@@ -23,7 +23,7 @@ module ActionView #:nodoc:
 
     private
 
-      def query(path, exts, _, locals)
+      def query(path, exts, _, locals, cache:)
         query = +""
         EXTENSIONS.each do |ext, prefix|
           query << "(" << exts[ext].map { |e| e && Regexp.escape("#{prefix}#{e}") }.join("|") << "|)"
@@ -47,7 +47,7 @@ module ActionView #:nodoc:
   end
 
   class NullResolver < PathResolver
-    def query(path, exts, _, locals)
+    def query(path, exts, _, locals, cache:)
       handler, format, variant = extract_handler_and_format_and_variant(path)
       [ActionView::Template.new("Template generated by Null Resolver", path.virtual, handler, virtual_path: path.virtual, format: format, variant: variant, locals: locals)]
     end
diff --git a/actionview/lib/action_view/unbound_template.rb b/actionview/lib/action_view/unbound_template.rb
new file mode 100644
index 0000000000..db69b6d016
--- /dev/null
+++ b/actionview/lib/action_view/unbound_template.rb
@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+
+require "concurrent/map"
+
+module ActionView
+  class UnboundTemplate
+    def initialize(source, identifer, handler, options)
+      @source = source
+      @identifer = identifer
+      @handler = handler
+      @options = options
+
+      @templates = Concurrent::Map.new(initial_capacity: 2)
+    end
+
+    def bind_locals(locals)
+      @templates[locals] ||= build_template(locals)
+    end
+
+    private
+
+      def build_template(locals)
+        options = @options.merge(locals: locals)
+        Template.new(
+          @source,
+          @identifer,
+          @handler,
+          options
+        )
+      end
+  end
+end
diff --git a/actionview/test/template/file_system_resolver_test.rb b/actionview/test/template/file_system_resolver_test.rb
new file mode 100644
index 0000000000..aa03fdcb13
--- /dev/null
+++ b/actionview/test/template/file_system_resolver_test.rb
@@ -0,0 +1,12 @@
+# frozen_string_literal: true
+
+require "abstract_unit"
+require "template/resolver_shared_tests"
+
+class FileSystemResolverTest < ActiveSupport::TestCase
+  include ResolverSharedTests
+
+  def resolver
+    ActionView::FileSystemResolver.new(tmpdir)
+  end
+end
diff --git a/actionview/test/template/optimized_file_system_resolver_test.rb b/actionview/test/template/optimized_file_system_resolver_test.rb
new file mode 100644
index 0000000000..c0c64357ce
--- /dev/null
+++ b/actionview/test/template/optimized_file_system_resolver_test.rb
@@ -0,0 +1,12 @@
+# frozen_string_literal: true
+
+require "abstract_unit"
+require "template/resolver_shared_tests"
+
+class OptimizedFileSystemResolverTest < ActiveSupport::TestCase
+  include ResolverSharedTests
+
+  def resolver
+    ActionView::OptimizedFileSystemResolver.new(tmpdir)
+  end
+end
diff --git a/actionview/test/template/resolver_shared_tests.rb b/actionview/test/template/resolver_shared_tests.rb
new file mode 100644
index 0000000000..525b452075
--- /dev/null
+++ b/actionview/test/template/resolver_shared_tests.rb
@@ -0,0 +1,146 @@
+module ResolverSharedTests
+  attr_reader :tmpdir
+
+  def run(*args)
+    capture_exceptions do
+      Dir.mktmpdir(nil, __dir__) { |dir| @tmpdir = dir; super }
+    end
+  end
+
+  def with_file(filename, source="File at #{filename}")
+    path = File.join(tmpdir, filename)
+    FileUtils.mkdir_p(File.dirname(path))
+    File.write(path, source)
+  end
+
+  def context
+    @context ||= ActionView::LookupContext.new(resolver)
+  end
+
+  def test_can_find_with_no_extensions
+    with_file "test/hello_world", "Hello default!"
+
+    templates = resolver.find_all("hello_world", "test", false, locale: [:en], formats: [:html], variants: [:phone], handlers: [:erb])
+    assert_equal 1, templates.size
+    assert_equal "Hello default!",   templates[0].source
+    assert_equal "test/hello_world", templates[0].virtual_path
+    assert_nil templates[0].format
+    assert_nil templates[0].variant
+    assert_kind_of ActionView::Template::Handlers::Raw, templates[0].handler
+  end
+
+  def test_can_find_with_just_handler
+    with_file "test/hello_world.erb", "Hello erb!"
+
+    templates = resolver.find_all("hello_world", "test", false, locale: [:en], formats: [:html], variants: [:phone], handlers: [:erb])
+    assert_equal 1, templates.size
+    assert_equal "Hello erb!",   templates[0].source
+    assert_equal "test/hello_world", templates[0].virtual_path
+    assert_nil templates[0].format
+    assert_nil templates[0].variant
+    assert_kind_of ActionView::Template::Handlers::ERB, templates[0].handler
+  end
+
+  def test_can_find_with_format_and_handler
+    with_file "test/hello_world.text.builder", "Hello plain text!"
+
+    templates = resolver.find_all("hello_world", "test", false, locale: [:en], formats: [:html, :text], variants: [:phone], handlers: [:erb, :builder])
+    assert_equal 1, templates.size
+    assert_equal "Hello plain text!", templates[0].source
+    assert_equal "test/hello_world",  templates[0].virtual_path
+    assert_equal :text, templates[0].format
+    assert_nil templates[0].variant
+    assert_kind_of ActionView::Template::Handlers::Builder, templates[0].handler
+  end
+
+  def test_can_find_with_variant_format_and_handler
+    with_file "test/hello_world.html+phone.erb", "Hello plain text!"
+
+    templates = resolver.find_all("hello_world", "test", false, locale: [:en], formats: [:html], variants: [:phone], handlers: [:erb])
+    assert_equal 1, templates.size
+    assert_equal "Hello plain text!", templates[0].source
+    assert_equal "test/hello_world",  templates[0].virtual_path
+    assert_equal :html, templates[0].format
+    assert_equal "phone", templates[0].variant
+    assert_kind_of ActionView::Template::Handlers::ERB, templates[0].handler
+  end
+
+  def test_can_find_with_any_variant_format_and_handler
+    with_file "test/hello_world.html+phone.erb", "Hello plain text!"
+
+    templates = resolver.find_all("hello_world", "test", false, locale: [:en], formats: [:html], variants: :any, handlers: [:erb])
+    assert_equal 1, templates.size
+    assert_equal "Hello plain text!", templates[0].source
+    assert_equal "test/hello_world",  templates[0].virtual_path
+    assert_equal :html, templates[0].format
+    assert_equal "phone", templates[0].variant
+    assert_kind_of ActionView::Template::Handlers::ERB, templates[0].handler
+  end
+
+  def test_doesnt_find_template_with_wrong_details
+    with_file "test/hello_world.html.erb", "Hello plain text!"
+
+    templates = resolver.find_all("hello_world", "test", false, locale: [], formats: [:xml], variants: :any, handlers: [:builder])
+    assert_equal 0, templates.size
+
+    templates = resolver.find_all("hello_world", "test", false, locale: [], formats: [:xml], variants: :any, handlers: [:erb])
+    assert_equal 0, templates.size
+  end
+
+  def test_found_template_is_cached
+    with_file "test/hello_world.html.erb", "Hello HTML!"
+
+    a = context.find("hello_world", "test", false, [], {})
+    b = context.find("hello_world", "test", false, [], {})
+    assert_same a, b
+  end
+
+  def test_different_templates_when_cache_disabled
+    with_file "test/hello_world.html.erb", "Hello HTML!"
+
+    a = context.find("hello_world", "test", false, [], {})
+    b = context.disable_cache { context.find("hello_world", "test", false, [], {}) }
+    c = context.find("hello_world", "test", false, [], {})
+
+    # disable_cache should give us a new object
+    refute_same a, b
+
+    # but it should not clear the cache
+    assert_same a, c
+  end
+
+  def test_same_template_from_different_details_is_same_object
+    with_file "test/hello_world.html.erb", "Hello HTML!"
+
+    a = context.find("hello_world", "test", false, [], locale: [:en])
+    b = context.find("hello_world", "test", false, [], locale: [:fr])
+    assert_same a, b
+  end
+
+  def test_templates_with_optional_locale_shares_common_object
+    with_file "test/hello_world.text.erb", "Generic plain text!"
+    with_file "test/hello_world.fr.text.erb", "Texte en Francais!"
+
+    en = context.find_all("hello_world", "test", false, [], locale: [:en])
+    fr = context.find_all("hello_world", "test", false, [], locale: [:fr])
+
+    assert_equal 1, en.size
+    assert_equal 2, fr.size
+
+    assert_equal "Generic plain text!", en[0].source
+    assert_equal "Texte en Francais!",  fr[0].source
+    assert_equal "Generic plain text!", fr[1].source
+
+    assert_same en[0], fr[1]
+  end
+
+  def test_virtual_path_is_preserved_with_dot
+    with_file "test/hello_world.html.erb", "Hello html!"
+
+    template = context.find("hello_world.html", "test", false, [], {})
+    assert_equal "test/hello_world.html", template.virtual_path
+
+    template = context.find("hello_world", "test", false, [], {})
+    assert_equal "test/hello_world", template.virtual_path
+  end
+end