Merge pull request #1553 from Shopify/catlee/shared_partial_cache

Ensure that partial caches are shared with subcontexts

History.md

@@ -1,5 +1,14 @@
 # Liquid Change Log
 
+## 5.3.1 (unreleased)
+
+### Fixes
+* `PartialCache` now shares snippet cache with subcontexts by default (#1553) [Chris AtLee]
+* Hash registers no longer leak into subcontexts as static registers (#1564) [Chris AtLee]
+
+### Changed
+* Liquid::Context#registers now always returns a Liquid::StaticRegisters object, though supports the most used Hash functions for compatibility (#1553)
+
 ## 5.3.0 2022-03-22
 
 ### Fixes

lib/liquid/context.rb

@@ -28,7 +28,7 @@ module Liquid
 
       @static_environments = [static_environments].flat_map(&:freeze).freeze
       @scopes              = [(outer_scope || {})]
-      @registers           = registers
+      @registers           = registers.is_a?(StaticRegisters) ? registers : StaticRegisters.new(registers)
       @errors              = []
       @partial             = false
       @strict_variables    = false
@@ -39,6 +39,10 @@ module Liquid
       @global_filter       = nil
       @disabled_tags       = {}
 
+      @registers.static[:cached_partials] ||= {}
+      @registers.static[:file_system] ||= Liquid::Template.file_system
+      @registers.static[:template_factory] ||= Liquid::TemplateFactory.new
+
       self.exception_renderer = Template.default_exception_renderer
       if rethrow_errors
         self.exception_renderer = Liquid::RAISE_EXCEPTION_LAMBDA

lib/liquid/partial_cache.rb

@@ -3,16 +3,16 @@
 module Liquid
   class PartialCache
     def self.load(template_name, context:, parse_context:)
-      cached_partials = (context.registers[:cached_partials] ||= {})
+      cached_partials = context.registers[:cached_partials]
       cached = cached_partials[template_name]
       return cached if cached
 
-      file_system = (context.registers[:file_system] ||= Liquid::Template.file_system)
+      file_system = context.registers[:file_system]
       source      = file_system.read_template_file(template_name)
 
       parse_context.partial = true
 
-      template_factory = (context.registers[:template_factory] ||= Liquid::TemplateFactory.new)
+      template_factory = context.registers[:template_factory]
       template = template_factory.for(template_name)
 
       partial = template.parse(source, parse_context)

lib/liquid/template.rb

@@ -167,15 +167,14 @@ module Liquid
 
       output = nil
 
-      context_register = context.registers.is_a?(StaticRegisters) ? context.registers.static : context.registers
-
       case args.last
       when Hash
         options = args.pop
         output  = options[:output] if options[:output]
+        static_registers = context.registers.static
 
         options[:registers]&.each do |key, register|
-          context_register[key] = register
+          static_registers[key] = register
         end
 
         apply_options_to_context(context, options)

test/integration/context_test.rb

@@ -618,6 +618,20 @@ class ContextTest < Minitest::Test
     end
   end
 
+  def test_context_always_uses_static_registers
+    registers = {
+      my_register: :my_value,
+    }
+    c = Context.new({}, {}, registers)
+    assert_instance_of(StaticRegisters, c.registers)
+    assert_equal(:my_value, c.registers[:my_register])
+
+    r = StaticRegisters.new(registers)
+    c = Context.new({}, {}, r)
+    assert_instance_of(StaticRegisters, c.registers)
+    assert_equal(:my_value, c.registers[:my_register])
+  end
+
   private
 
   def assert_no_object_allocations

test/integration/standard_filter_test.rb

@@ -32,7 +32,7 @@ class TestDrop < Liquid::Drop
   attr_reader :value
 
   def registers
-    @context.registers
+    { @value => @context.registers[@value] }
   end
 end
 
@@ -440,7 +440,7 @@ class StandardFiltersTest < Minitest::Test
   end
 
   def test_map_calls_context=
-    model = TestModel.new(value: "test")
+    model = TestModel.new(value: :test)
 
     template = Template.parse('{{ foo | map: "registers" }}')
     template.registers[:test] = 1234

test/unit/partial_cache_unit_test.rb

@@ -125,4 +125,35 @@ class PartialCacheUnitTest < Minitest::Test
     assert_equal('my partial body', partial.render)
     assert_equal(1, template_factory.count)
   end
+
+  def test_cache_state_is_shared_for_subcontexts
+    parse_context      = Liquid::ParseContext.new
+    shared_file_system = StubFileSystem.new(
+      'my_partial' => 'my shared value'
+    )
+    context = Liquid::Context.build(
+      registers: Liquid::StaticRegisters.new(
+        file_system: shared_file_system,
+      )
+    )
+    subcontext = context.new_isolated_subcontext
+
+    assert_equal(subcontext.registers[:cached_partials].object_id, context.registers[:cached_partials].object_id)
+
+    2.times do
+      Liquid::PartialCache.load(
+        'my_partial',
+        context: context,
+        parse_context: parse_context
+      )
+
+      Liquid::PartialCache.load(
+        'my_partial',
+        context: subcontext,
+        parse_context: parse_context
+      )
+    end
+
+    assert_equal(1, shared_file_system.file_read_count)
+  end
 end