From c0ffee4133e26ac07674d0c65212561edfb5377f Mon Sep 17 00:00:00 2001 From: Thierry Joyal Date: Wed, 15 Jan 2020 14:14:04 +0000 Subject: [PATCH] [StrainerTemplate] Isolate filter mods --- lib/liquid.rb | 2 + lib/liquid/filter.rb | 43 +++++++++++++++ lib/liquid/filter_template.rb | 40 ++++++++++++++ lib/liquid/strainer_template.rb | 51 +++++++++++++----- test/unit/context_unit_test.rb | 6 +-- test/unit/strainer_factory_unit_test.rb | 10 ---- test/unit/strainer_template_unit_test.rb | 68 +----------------------- 7 files changed, 128 insertions(+), 92 deletions(-) create mode 100644 lib/liquid/filter.rb create mode 100644 lib/liquid/filter_template.rb diff --git a/lib/liquid.rb b/lib/liquid.rb index 84b11e5..aa9cbe8 100644 --- a/lib/liquid.rb +++ b/lib/liquid.rb @@ -57,6 +57,8 @@ require 'liquid/forloop_drop' require 'liquid/extensions' require 'liquid/errors' require 'liquid/interrupts' +require 'liquid/filter' +require 'liquid/filter_template' require 'liquid/strainer_factory' require 'liquid/strainer_template' require 'liquid/expression' diff --git a/lib/liquid/filter.rb b/lib/liquid/filter.rb new file mode 100644 index 0000000..8771212 --- /dev/null +++ b/lib/liquid/filter.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'set' + +module Liquid + # A filter in liquid is a class which contain invokable logic from liquid templates. + # + # Public methods in filter classes are callable. + # + # The use for liquid filters is to make logic functions available to the web designers. + # + # Example: + # + # class StringFilter < Liquid::Filter + # def upcase(input) + # input.upcase + # end + # end + # + # tmpl = Liquid::Template.parse('Result: {{ "test" | upcase }}') + # tmpl.render({}, filters: [StringFilter]) + # => "Result: TEST" + class Filter + class << self + def invokable_methods + @invokable_methods ||= begin + blacklist = Liquid::Filter.public_instance_methods + whitelist = public_instance_methods - blacklist + + Set.new(whitelist.map(&:to_s)) + end + end + end + + def initialize(context) + @context = context + end + + private + + attr_reader :context + end +end diff --git a/lib/liquid/filter_template.rb b/lib/liquid/filter_template.rb new file mode 100644 index 0000000..defdb63 --- /dev/null +++ b/lib/liquid/filter_template.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +require 'set' + +module Liquid + # FilterTemplate is the computed class for the filters system. + # + # Historically Liquid used to include filters as Module to the context strainer. + # This lead to the absence of sandbox between filters (one filter could override private methods of another filter). + # + # With the implementation of Liquid::Filter, it is now possible for the modules from legacy code to be automatically + # wrapped into a Liquid::Filter generated class. + # + # This should not be considered as the base behaviour, it is preferred to create filters going forward directly as + # classes that are child of Liquid::Filter. + class FilterTemplate < Filter + class << self + def include(mod) + super + + @init_module = mod + end + + # Override of the `invokable_methods`. + # We can't rely on the parent logic as some modules might have been defining methods that shadow Class methods. + # + # Eg.: + # mod = Liquid::StandardFilters + # filter = Class.new(FilterTemplate) + # filter.include(mod) + # mod.public_instance_methods - (filter.public_instance_methods - Class.public_instance_methods) + # => [:prepend] + def invokable_methods + whitelist = @init_module.public_instance_methods + + @invokable_methods ||= Set.new(whitelist.map(&:to_s)) + end + end + end +end diff --git a/lib/liquid/strainer_template.rb b/lib/liquid/strainer_template.rb index a3ad06a..1d71bce 100644 --- a/lib/liquid/strainer_template.rb +++ b/lib/liquid/strainer_template.rb @@ -14,40 +14,65 @@ module Liquid end class << self - def add_filter(filter) - return if include?(filter) - - invokable_non_public_methods = (filter.private_instance_methods + filter.protected_instance_methods).select { |m| invokable?(m) } - if invokable_non_public_methods.any? - raise MethodOverrideError, "Filter overrides registered public methods as non public: #{invokable_non_public_methods.join(', ')}" + def add_filter(mod) + filter = if mod.is_a?(Class) && mod.ancestors.include?(Liquid::Filter) + mod + elsif mod.instance_of?(Module) + convert_mod_to_filter(mod) + else + raise(ArgumentError, "wrong argument type Proc (expected Liquid::Filter)") end - include(filter) + filter.invokable_methods.each do |method| + filter_map[method] = filter + end + end - filter_methods.merge(filter.public_instance_methods.map(&:to_s)) + def fetch_filter(method) + filter_map.fetch(method) end def invokable?(method) - filter_methods.include?(method.to_s) + filter_map.key?(method) end private - def filter_methods - @filter_methods ||= Set.new + def filter_map + @filter_map ||= {} + end + + def convert_mod_to_filter(mod) + @filter_classes ||= {} + @filter_classes[mod] ||= begin + klass = Class.new(FilterTemplate) + klass.include(mod) + klass + end + end + + def filter_class_by_methods + @filter_class_by_methods ||= {} end end def invoke(method, *args) if self.class.invokable?(method) - send(method, *args) + klass = self.class.fetch_filter(method) + + instance = klass.new(@context) + instance.public_send(method, *args) elsif @context.strict_filters - raise Liquid::UndefinedFilter, "undefined filter #{method}" + raise(Liquid::UndefinedFilter, "undefined filter #{method}") else args.first end rescue ::ArgumentError => e raise Liquid::ArgumentError, e.message, e.backtrace end + + private + + attr_reader :context end end diff --git a/test/unit/context_unit_test.rb b/test/unit/context_unit_test.rb index c57bbca..854704f 100644 --- a/test/unit/context_unit_test.rb +++ b/test/unit/context_unit_test.rb @@ -147,13 +147,13 @@ class ContextUnitTest < Minitest::Test context = Context.new context.add_filters(filter) - assert_equal('hi? hi!', context.invoke(:hi, 'hi?')) + assert_equal('hi? hi!', context.invoke('hi', 'hi?')) context = Context.new - assert_equal('hi?', context.invoke(:hi, 'hi?')) + assert_equal('hi?', context.invoke('hi', 'hi?')) context.add_filters(filter) - assert_equal('hi? hi!', context.invoke(:hi, 'hi?')) + assert_equal('hi? hi!', context.invoke('hi', 'hi?')) end def test_only_intended_filters_make_it_there diff --git a/test/unit/strainer_factory_unit_test.rb b/test/unit/strainer_factory_unit_test.rb index 05a9c83..427cbf0 100644 --- a/test/unit/strainer_factory_unit_test.rb +++ b/test/unit/strainer_factory_unit_test.rb @@ -82,16 +82,6 @@ class StrainerFactoryUnitTest < Minitest::Test assert_equal("has_method?", strainer.invoke("invoke", "has_method?", "invoke")) end - def test_strainer_uses_a_class_cache_to_avoid_method_cache_invalidation - a = Module.new - b = Module.new - strainer = StrainerFactory.create(@context, [a, b]) - assert_kind_of(StrainerTemplate, strainer) - assert_kind_of(a, strainer) - assert_kind_of(b, strainer) - assert_kind_of(Liquid::StandardFilters, strainer) - end - def test_add_global_filter_clears_cache assert_equal('input', StrainerFactory.create(@context).invoke('late_added_filter', 'input')) StrainerFactory.add_global_filter(LateAddedFilter) diff --git a/test/unit/strainer_template_unit_test.rb b/test/unit/strainer_template_unit_test.rb index e72ed44..a2a7761 100644 --- a/test/unit/strainer_template_unit_test.rb +++ b/test/unit/strainer_template_unit_test.rb @@ -10,73 +10,9 @@ class StrainerTemplateUnitTest < Minitest::Test s = c.strainer wrong_filter = ->(v) { v.reverse } - exception = assert_raises(TypeError) do + exception = assert_raises(ArgumentError) do s.class.add_filter(wrong_filter) end - assert_equal(exception.message, "wrong argument type Proc (expected Module)") - end - - module PrivateMethodOverrideFilter - private - - def public_filter - "overriden as private" - end - end - - def test_add_filter_raises_when_module_privately_overrides_registered_public_methods - strainer = Context.new.strainer - - error = assert_raises(Liquid::MethodOverrideError) do - strainer.class.add_filter(PrivateMethodOverrideFilter) - end - assert_equal('Liquid error: Filter overrides registered public methods as non public: public_filter', error.message) - end - - module ProtectedMethodOverrideFilter - protected - - def public_filter - "overriden as protected" - end - end - - def test_add_filter_raises_when_module_overrides_registered_public_method_as_protected - strainer = Context.new.strainer - - error = assert_raises(Liquid::MethodOverrideError) do - strainer.class.add_filter(ProtectedMethodOverrideFilter) - end - assert_equal('Liquid error: Filter overrides registered public methods as non public: public_filter', error.message) - end - - module PublicMethodOverrideFilter - def public_filter - "public" - end - end - - def test_add_filter_does_not_raise_when_module_overrides_previously_registered_method - strainer = Context.new.strainer - with_global_filter do - strainer.class.add_filter(PublicMethodOverrideFilter) - assert(strainer.class.send(:filter_methods).include?('public_filter')) - end - end - - def test_add_filter_does_not_include_already_included_module - mod = Module.new do - class << self - attr_accessor :include_count - def included(_mod) - self.include_count += 1 - end - end - self.include_count = 0 - end - strainer = Context.new.strainer - strainer.class.add_filter(mod) - strainer.class.add_filter(mod) - assert_equal(1, mod.include_count) + assert_equal(exception.message, "Liquid error: wrong argument type Proc (expected Liquid::Filter)") end end