Merge pull request #705 from Shopify/register-filter-warn

Strainer#add_filter Raises on Private Override

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]

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

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_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))
+        end
       end
     end
 

test/unit/strainer_unit_test.rb

@@ -87,4 +87,50 @@ 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 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
+    strainer.class.add_filter(PublicMethodOverrideFilter)
+    assert strainer.class.filter_methods.include?('public_filter')
+  end
 end # StrainerTest