From 80e2aaa80afc205d223288b85828729cb181f6f2 Mon Sep 17 00:00:00 2001
From: John Hawthorn <john@hawthorn.email>
Date: Mon, 18 Mar 2019 12:03:39 -0700
Subject: Add tests against resolver

We didn't previously have many tests directly against the
OptimizedFileSystemResolver or FileSystemResolver, though usually
failures would be exposed through other tests.

It's easier to test some specifics of the behaviour with unit tests.
This also lets us test FileSystemResolver (non-optimized) which I don't
think previously had much testing (other than from classses inheriting
it).
---
 .../test/template/file_system_resolver_test.rb     | 12 +++
 .../optimized_file_system_resolver_test.rb         | 12 +++
 actionview/test/template/resolver_shared_tests.rb  | 85 ++++++++++++++++++++++
 3 files changed, 109 insertions(+)
 create mode 100644 actionview/test/template/file_system_resolver_test.rb
 create mode 100644 actionview/test/template/optimized_file_system_resolver_test.rb
 create mode 100644 actionview/test/template/resolver_shared_tests.rb

(limited to 'actionview')

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..dee6a69c66
--- /dev/null
+++ b/actionview/test/template/resolver_shared_tests.rb
@@ -0,0 +1,85 @@
+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 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
+end
-- 
cgit v1.2.3


From 1fc735e5f584b481eba85670c519731271ac1796 Mon Sep 17 00:00:00 2001
From: John Hawthorn <john@hawthorn.email>
Date: Thu, 11 Apr 2019 17:14:16 -0700
Subject: De-dup Templates, introduce UnboundTemplate

Previously it's possible to have multiple copies of the "same" Template.
For example, if index.html.erb is found both the :en and :fr locale, it
will return a different Template object for each. The same can happen
with formats, variants, and handlers.

This commit de-duplicates templates, there will now only be one template
per file/virtual_path/locals tuple.

We need to consider virtual_path because both `render "index"`, and
`render "index.html"` can both find the same file but will have
different virtual_paths. IMO this is rare and should be
deprecated/removed, but it exists now so we need to consider it in order
to cache correctly.

This commit introduces a new UnboundTemplate class, which represents a
template with unknown locals. Template objects can be built from it by
using `#with_locals`. Currently, this is just a convenience around
caching templates, but I hope it's a helpful concept that could have
more utility in the future.
---
 actionview/lib/action_view.rb                     |  1 +
 actionview/lib/action_view/template/resolver.rb   | 29 +++++++++++++-------
 actionview/lib/action_view/unbound_template.rb    | 32 +++++++++++++++++++++++
 actionview/test/template/resolver_shared_tests.rb | 30 +++++++++++++++++++++
 4 files changed, 82 insertions(+), 10 deletions(-)
 create mode 100644 actionview/lib/action_view/unbound_template.rb

(limited to 'actionview')

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..1be82f5df5 100644
--- a/actionview/lib/action_view/template/resolver.rb
+++ b/actionview/lib/action_view/template/resolver.rb
@@ -169,6 +169,12 @@ module ActionView
       else
         @pattern = DEFAULT_PATTERN
       end
+      @unbound_templates = Concurrent::Map.new
+      super()
+    end
+
+    def clear_cache
+      @unbound_templates.clear
       super()
     end
 
@@ -189,16 +195,19 @@ module ActionView
       end
 
       def build_template(template, virtual_path, locals)
-        handler, format, variant = extract_handler_and_format_and_variant(template)
-
-        filename = File.expand_path(template)
-        source = Template::Sources::File.new(filename)
-        Template.new(source, filename, handler,
-          virtual_path: virtual_path,
-          format: format,
-          variant: variant,
-          locals: locals
-        )
+        @unbound_templates.compute_if_absent([template, virtual_path]) do
+          handler, format, variant = extract_handler_and_format_and_variant(template)
+          source = Template::Sources::File.new(template)
+
+          UnboundTemplate.new(
+            source,
+            template,
+            handler,
+            virtual_path: virtual_path,
+            format: format,
+            variant: variant,
+          )
+        end.bind_locals(locals)
       end
 
       def reject_files_external_to_app(files)
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/resolver_shared_tests.rb b/actionview/test/template/resolver_shared_tests.rb
index dee6a69c66..37bdf4b22c 100644
--- a/actionview/test/template/resolver_shared_tests.rb
+++ b/actionview/test/template/resolver_shared_tests.rb
@@ -13,6 +13,10 @@ module ResolverSharedTests
     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!"
 
@@ -82,4 +86,30 @@ module ResolverSharedTests
     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_same_template_from_different_details_is_same_object
+    with_file "test/hello_world.html.erb", "Hello plain text!"
+
+    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_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
-- 
cgit v1.2.3


From 53e4055a75fc16bf31aeb295ec18c07a3059db57 Mon Sep 17 00:00:00 2001
From: John Hawthorn <john@hawthorn.email>
Date: Fri, 12 Apr 2019 10:20:04 -0700
Subject: Support disabling cache for Digestor

This adds a bit of complexity, but is necessary for now to avoid holding
extra copies of templates which are resolved from ActionView::Digestor
after disabling cache on the lookup context.
---
 actionview/lib/action_view/template/resolver.rb   | 55 ++++++++++++++---------
 actionview/lib/action_view/testing/resolvers.rb   |  4 +-
 actionview/test/template/resolver_shared_tests.rb | 14 ++++++
 3 files changed, 51 insertions(+), 22 deletions(-)

(limited to 'actionview')

diff --git a/actionview/lib/action_view/template/resolver.rb b/actionview/lib/action_view/template/resolver.rb
index 1be82f5df5..6a9beede8c 100644
--- a/actionview/lib/action_view/template/resolver.rb
+++ b/actionview/lib/action_view/template/resolver.rb
@@ -178,36 +178,51 @@ module ActionView
       super()
     end
 
+    def find_all(name, prefix = nil, partial = false, details = {}, key = nil, locals = [])
+      locals = locals.map(&:to_s).sort!.freeze
+
+      cached(key, [name, prefix, partial], details, locals) do
+        find_templates(name, prefix, partial, details, locals, cache: !!key)
+      end
+    end
+
     private
 
-      def find_templates(name, prefix, partial, details, locals)
+      def find_templates(name, prefix, partial, details, locals, cache: true)
         path = Path.build(name, prefix, partial)
-        query(path, details, details[:formats], locals)
+        query(path, details, details[:formats], locals, cache: cache)
       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)
-        @unbound_templates.compute_if_absent([template, virtual_path]) do
-          handler, format, variant = extract_handler_and_format_and_variant(template)
-          source = Template::Sources::File.new(template)
-
-          UnboundTemplate.new(
-            source,
-            template,
-            handler,
-            virtual_path: virtual_path,
-            format: format,
-            variant: variant,
-          )
-        end.bind_locals(locals)
+      def build_unbound_template(template, virtual_path)
+        handler, format, variant = extract_handler_and_format_and_variant(template)
+        source = Template::Sources::File.new(template)
+
+        UnboundTemplate.new(
+          source,
+          template,
+          handler,
+          virtual_path: virtual_path,
+          format: format,
+          variant: variant,
+        )
       end
 
       def reject_files_external_to_app(files)
@@ -372,8 +387,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/test/template/resolver_shared_tests.rb b/actionview/test/template/resolver_shared_tests.rb
index 37bdf4b22c..ee73ec1922 100644
--- a/actionview/test/template/resolver_shared_tests.rb
+++ b/actionview/test/template/resolver_shared_tests.rb
@@ -95,6 +95,20 @@ module ResolverSharedTests
     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 plain text!"
 
-- 
cgit v1.2.3


From 0b26cc41dbd8c57eb382318a63c4272de3d2e4e9 Mon Sep 17 00:00:00 2001
From: John Hawthorn <john@hawthorn.email>
Date: Fri, 12 Apr 2019 15:40:33 -0700
Subject: Avoid duplication using _find_all

---
 actionview/lib/action_view/template/resolver.rb | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

(limited to 'actionview')

diff --git a/actionview/lib/action_view/template/resolver.rb b/actionview/lib/action_view/template/resolver.rb
index 6a9beede8c..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
@@ -178,19 +182,11 @@ module ActionView
       super()
     end
 
-    def find_all(name, prefix = nil, partial = false, details = {}, key = nil, locals = [])
-      locals = locals.map(&:to_s).sort!.freeze
-
-      cached(key, [name, prefix, partial], details, locals) do
-        find_templates(name, prefix, partial, details, locals, cache: !!key)
-      end
-    end
-
     private
 
-      def find_templates(name, prefix, partial, details, locals, cache: true)
+      def _find_all(name, prefix, partial, details, key, locals)
         path = Path.build(name, prefix, partial)
-        query(path, details, details[:formats], locals, cache: cache)
+        query(path, details, details[:formats], locals, cache: !!key)
       end
 
       def query(path, details, formats, locals, cache:)
-- 
cgit v1.2.3


From 2455d16a5a975fa5bf75bdb023df61047b055e3d Mon Sep 17 00:00:00 2001
From: John Hawthorn <john@hawthorn.email>
Date: Fri, 12 Apr 2019 16:08:30 -0700
Subject: Add additional test for sharing templates

---
 actionview/test/template/resolver_shared_tests.rb | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

(limited to 'actionview')

diff --git a/actionview/test/template/resolver_shared_tests.rb b/actionview/test/template/resolver_shared_tests.rb
index ee73ec1922..525b452075 100644
--- a/actionview/test/template/resolver_shared_tests.rb
+++ b/actionview/test/template/resolver_shared_tests.rb
@@ -110,13 +110,30 @@ module ResolverSharedTests
   end
 
   def test_same_template_from_different_details_is_same_object
-    with_file "test/hello_world.html.erb", "Hello plain text!"
+    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!"
 
-- 
cgit v1.2.3