From 8be38d17956b513a420fd78002756fe391016af1 Mon Sep 17 00:00:00 2001 From: Gaurav Chande Date: Wed, 24 Feb 2016 19:38:30 +0000 Subject: [PATCH 1/3] Strainer#add_filter should raise when registered method is overriden as private --- lib/liquid/errors.rb | 1 + lib/liquid/strainer.rb | 9 +++++++-- test/unit/strainer_unit_test.rb | 29 +++++++++++++++++++++++++++++ 3 files changed, 37 insertions(+), 2 deletions(-) diff --git a/lib/liquid/errors.rb b/lib/liquid/errors.rb index 762828c..6dcd05e 100644 --- a/lib/liquid/errors.rb +++ b/lib/liquid/errors.rb @@ -59,4 +59,5 @@ module Liquid UndefinedVariable = Class.new(Error) UndefinedDropMethod = Class.new(Error) UndefinedFilter = Class.new(Error) + MethodOverrideError = Class.new(Error) end diff --git a/lib/liquid/strainer.rb b/lib/liquid/strainer.rb index c79a586..7ecb31a 100644 --- a/lib/liquid/strainer.rb +++ b/lib/liquid/strainer.rb @@ -28,8 +28,13 @@ module Liquid def self.add_filter(filter) raise ArgumentError, "Expected module but got: #{filter.class}" unless filter.is_a?(Module) unless self.class.include?(filter) - send(:include, filter) - @filter_methods.merge(filter.public_instance_methods.map(&:to_s)) + invokable_private_methods = filter.private_instance_methods.select { |m| invokable?(m) } + if invokable_private_methods.any? + raise MethodOverrideError, "Filter overrides registered public methods as private: #{invokable_private_methods.join(', ')}" + else + send(:include, filter) + @filter_methods.merge(filter.public_instance_methods.map(&:to_s)) + end end end diff --git a/test/unit/strainer_unit_test.rb b/test/unit/strainer_unit_test.rb index d06d30a..ef32c5f 100644 --- a/test/unit/strainer_unit_test.rb +++ b/test/unit/strainer_unit_test.rb @@ -87,4 +87,33 @@ class StrainerUnitTest < Minitest::Test s.class.add_filter(wrong_filter) end 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 private: 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 + strainer.class.add_filter(PublicMethodOverrideFilter) + assert strainer.class.filter_methods.include?('public_filter') + end end # StrainerTest From 8e6b9d503d207c315e7c7f59adb3ef4a63901228 Mon Sep 17 00:00:00 2001 From: Gaurav Chande Date: Wed, 24 Feb 2016 20:18:40 +0000 Subject: [PATCH 2/3] Make Strainer also raise when registered method is overriden as protected --- lib/liquid/strainer.rb | 6 +++--- test/unit/strainer_unit_test.rb | 19 ++++++++++++++++++- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/lib/liquid/strainer.rb b/lib/liquid/strainer.rb index 7ecb31a..441e335 100644 --- a/lib/liquid/strainer.rb +++ b/lib/liquid/strainer.rb @@ -28,9 +28,9 @@ module Liquid def self.add_filter(filter) raise ArgumentError, "Expected module but got: #{filter.class}" unless filter.is_a?(Module) unless self.class.include?(filter) - invokable_private_methods = filter.private_instance_methods.select { |m| invokable?(m) } - if invokable_private_methods.any? - raise MethodOverrideError, "Filter overrides registered public methods as private: #{invokable_private_methods.join(', ')}" + 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(', ')}" else send(:include, filter) @filter_methods.merge(filter.public_instance_methods.map(&:to_s)) diff --git a/test/unit/strainer_unit_test.rb b/test/unit/strainer_unit_test.rb index ef32c5f..5cdf2e8 100644 --- a/test/unit/strainer_unit_test.rb +++ b/test/unit/strainer_unit_test.rb @@ -102,7 +102,24 @@ class StrainerUnitTest < Minitest::Test error = assert_raises(Liquid::MethodOverrideError) do strainer.class.add_filter(PrivateMethodOverrideFilter) end - assert_equal 'Liquid error: Filter overrides registered public methods as private: public_filter', error.message + 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 From c424d47274bdd653a62b075e195d19c15d9fbfde Mon Sep 17 00:00:00 2001 From: Gaurav Chande Date: Wed, 24 Feb 2016 20:21:24 +0000 Subject: [PATCH 3/3] Add changelog entry for Strainer method override change --- History.md | 1 + 1 file changed, 1 insertion(+) diff --git a/History.md b/History.md index bc4949e..5772a03 100644 --- a/History.md +++ b/History.md @@ -18,6 +18,7 @@ * Ruby 1.9 support dropped (#491) [Justin Li] * Liquid::Template.file_system's read_template_file method is no longer passed the context. (#441) [James Reid-Smith] * Remove support for `liquid_methods` +* Liquid::Template.register_filter raises when the module overrides registered public methods as private or protected (#705) [Gaurav Chande] ### Fixed * Fix map filter when value is a Proc (#672) [Guillaume Malette]