Merge pull request #1059 from Shopify/map_error_checking

Apply error-checking to sort, sort_natural, where, uniq, map, compact filter(s)

lib/liquid/standardfilters.rb

@@ -128,8 +128,10 @@ module Liquid
           nil_safe_compare(a, b)
         end
       elsif ary.all? { |el| el.respond_to?(:[]) }
-        ary.sort do |a, b|
-          nil_safe_compare(a[property], b[property])
+        begin
+          ary.sort { |a, b| nil_safe_compare(a[property], b[property]) }
+        rescue TypeError
+          raise_property_error(property)
         end
       end
     end
@@ -146,8 +148,10 @@ module Liquid
           nil_safe_casecmp(a, b)
         end
       elsif ary.all? { |el| el.respond_to?(:[]) }
-        ary.sort do |a, b|
-          nil_safe_casecmp(a[property], b[property])
+        begin
+          ary.sort { |a, b| nil_safe_casecmp(a[property], b[property]) }
+        rescue TypeError
+          raise_property_error(property)
         end
       end
     end
@@ -160,9 +164,17 @@ module Liquid
       if ary.empty?
         []
       elsif ary.first.respond_to?(:[]) && target_value.nil?
-        ary.where_present(property)
+        begin
+          ary.select { |item| item[property] }
+        rescue TypeError
+          raise_property_error(property)
+        end
       elsif ary.first.respond_to?(:[])
-        ary.where(property, target_value)
+        begin
+          ary.select { |item| item[property] == target_value }
+        rescue TypeError
+          raise_property_error(property)
+        end
       end
     end
 
@@ -176,7 +188,11 @@ module Liquid
       elsif ary.empty? # The next two cases assume a non-empty array.
         []
       elsif ary.first.respond_to?(:[])
-        ary.uniq{ |a| a[property] }
+        begin
+          ary.uniq { |a| a[property] }
+        rescue TypeError
+          raise_property_error(property)
+        end
       end
     end
 
@@ -198,6 +214,8 @@ module Liquid
           r.is_a?(Proc) ? r.call : r
         end
       end
+    rescue TypeError
+      raise_property_error(property)
     end
 
     # Remove nils within an array
@@ -210,7 +228,11 @@ module Liquid
       elsif ary.empty? # The next two cases assume a non-empty array.
         []
       elsif ary.first.respond_to?(:[])
-        ary.reject{ |a| a[property].nil? }
+        begin
+          ary.reject { |a| a[property].nil? }
+        rescue TypeError
+          raise_property_error(property)
+        end
       end
     end
 
@@ -394,6 +416,10 @@ module Liquid
 
     private
 
+    def raise_property_error(property)
+      raise Liquid::ArgumentError.new("cannot select the property '#{property}'")
+    end
+
     def apply_operation(input, operand, operation)
       result = Utils.to_number(input).send(operation, Utils.to_number(operand))
       result.is_a?(BigDecimal) ? result.to_f : result
@@ -460,24 +486,6 @@ module Liquid
           yield(e.respond_to?(:to_liquid) ? e.to_liquid : e)
         end
       end
-
-      def where(property, target_value)
-        select do |item|
-          item[property] == target_value
-        end
-      rescue TypeError
-        # Cannot index with the given property type (eg. indexing integers with strings
-        # which are only allowed to be indexed by other integers).
-        raise ArgumentError.new("cannot select the property `#{property}`")
-      end
-
-      def where_present(property)
-        select { |item| item[property] }
-      rescue TypeError
-        # Cannot index with the given property type (eg. indexing integers with strings
-        # which are only allowed to be indexed by other integers).
-        raise ArgumentError.new("cannot select the property `#{property}`")
-      end
     end
   end
 

test/integration/standard_filter_test.rb

@@ -268,10 +268,34 @@ class StandardFiltersTest < Minitest::Test
     assert_equal [], @filters.sort([], "a")
   end
 
+  def test_sort_invalid_property
+    foo = [
+      [1],
+      [2],
+      [3]
+    ]
+
+    assert_raises Liquid::ArgumentError do
+      @filters.sort(foo, "bar")
+    end
+  end
+
   def test_sort_natural_empty_array
     assert_equal [], @filters.sort_natural([], "a")
   end
 
+  def test_sort_natural_invalid_property
+    foo = [
+      [1],
+      [2],
+      [3]
+    ]
+
+    assert_raises Liquid::ArgumentError do
+      @filters.sort_natural(foo, "bar")
+    end
+  end
+
   def test_legacy_sort_hash
     assert_equal [{ a: 1, b: 2 }], @filters.sort({ a: 1, b: 2 })
   end
@@ -295,10 +319,34 @@ class StandardFiltersTest < Minitest::Test
     assert_equal [], @filters.uniq([], "a")
   end
 
+  def test_uniq_invalid_property
+    foo = [
+      [1],
+      [2],
+      [3]
+    ]
+
+    assert_raises Liquid::ArgumentError do
+      @filters.uniq(foo, "bar")
+    end
+  end
+
   def test_compact_empty_array
     assert_equal [], @filters.compact([], "a")
   end
 
+  def test_compact_invalid_property
+    foo = [
+      [1],
+      [2],
+      [3]
+    ]
+
+    assert_raises Liquid::ArgumentError do
+      @filters.compact(foo, "bar")
+    end
+  end
+
   def test_reverse
     assert_equal [4, 3, 2, 1], @filters.reverse([1, 2, 3, 4])
   end
@@ -364,6 +412,29 @@ class StandardFiltersTest < Minitest::Test
     assert_template_result "123", '{{ foo | map: "foo" }}', "foo" => TestEnumerable.new
   end
 
+  def test_map_returns_empty_on_2d_input_array
+    foo = [
+      [1],
+      [2],
+      [3]
+    ]
+
+    assert_raises Liquid::ArgumentError do
+      @filters.map(foo, "bar")
+    end
+  end
+
+  def test_map_returns_empty_with_no_property
+    foo = [
+      [1],
+      [2],
+      [3]
+    ]
+    assert_raises Liquid::ArgumentError do
+      @filters.map(foo, nil)
+    end
+  end
+
   def test_sort_works_on_enumerables
     assert_template_result "213", '{{ foo | sort: "bar" | map: "foo" }}', "foo" => TestEnumerable.new
   end