From 0ae38bac696b457fcf20481fd38f8b93e2bc1be3 Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Wed, 29 Jan 2025 16:50:05 +0100 Subject: [PATCH] Fix backward-compatibility issue with the 'sort' filter --- History.md | 4 ++ lib/liquid/standardfilters.rb | 34 ++++++++- lib/liquid/version.rb | 2 +- test/integration/standard_filter_test.rb | 88 ++++++++++++++++++++++++ 4 files changed, 125 insertions(+), 3 deletions(-) diff --git a/History.md b/History.md index 87635f6a..108c919c 100644 --- a/History.md +++ b/History.md @@ -2,6 +2,10 @@ ## 5.8.0 (unreleased) +## 5.7.2 2025-01-30 + +- Fix the `sort` filter to handle nested properties gracefully when their types don't match + ## 5.7.1 2025-01-24 * Fix the `find` and `find_index`filters to return `nil` when filtering empty arrays diff --git a/lib/liquid/standardfilters.rb b/lib/liquid/standardfilters.rb index aee01184..9e532705 100644 --- a/lib/liquid/standardfilters.rb +++ b/lib/liquid/standardfilters.rb @@ -387,7 +387,23 @@ module Liquid end elsif ary.all? { |el| el.respond_to?(:[]) } begin - ary.sort { |a, b| nil_safe_compare(fetch_property(a, property), fetch_property(b, property)) } + ary.sort do |a, b| + a = fetch_property(a, property) + b = fetch_property(b, property) + + ## + # We handle nested properties gracefully to avoid breaking backward + # compatibility. + # + # However, we raise errors for incompatible types when no nested + # properties are used to maintain strict type checking in simple + # cases. + if has_nested_property?(property) + type_safe_compare(a, b) { |a, b| nil_safe_compare(a, b) } + else + nil_safe_compare(a, b) + end + end rescue TypeError raise_property_error(property) end @@ -1005,7 +1021,7 @@ module Liquid # ``` value = drop[property_or_keys] - return value if !value.nil? || !property_or_keys.is_a?(String) + return value if !value.nil? || !has_nested_property?(property_or_keys) keys = property_or_keys.split('.') keys.reduce(drop) do |drop, key| @@ -1013,6 +1029,10 @@ module Liquid end end + def has_nested_property?(property) + property.is_a?(String) && property.include?('.') + end + def raise_property_error(property) raise Liquid::ArgumentError, "cannot select the property '#{property}'" end @@ -1036,6 +1056,16 @@ module Liquid end end + def type_safe_compare(a, b) + klass_a = a.class + klass_b = b.class + + # Converting classes to string to have a deterministic comparison. + return nil_safe_casecmp(klass_a, klass_b) if klass_a != klass_b + + yield(a, b) + end + def nil_safe_casecmp(a, b) if !a.nil? && !b.nil? a.to_s.casecmp(b.to_s) diff --git a/lib/liquid/version.rb b/lib/liquid/version.rb index 8cfd6e80..205d2080 100644 --- a/lib/liquid/version.rb +++ b/lib/liquid/version.rb @@ -2,5 +2,5 @@ # frozen_string_literal: true module Liquid - VERSION = "5.7.1" + VERSION = "5.7.2" end diff --git a/test/integration/standard_filter_test.rb b/test/integration/standard_filter_test.rb index 0ef8e2bc..80eb8f6b 100644 --- a/test/integration/standard_filter_test.rb +++ b/test/integration/standard_filter_test.rb @@ -1385,6 +1385,94 @@ class StandardFiltersTest < Minitest::Test assert_template_result(expected_output, template, { "products" => TestDeepEnumerable.new }) end + def test_sort_with_different_types + input = [ + { "price" => 1000 }, + { "price" => :none }, + { "price" => 3000 } + ] + + assert_raises(Liquid::ArgumentError) do + @filters.sort(input, "price") + end + end + + def test_sort_with_nested_different_types + input = [ + { "price" => { "value" => 1000, "unit" => "BRL" } }, + { "price" => { "value" => 2000, "unit" => nil } }, + { "price" => { "value" => 3000, "unit" => :none } } + ] + expected_output = "2000, 1000, 3000" + template = <<~LIQUID + {{- input | sort: 'price.unit' | map: 'price.value' | join: ', ' -}} + LIQUID + + assert_template_result(expected_output, template, { "input" => input }) + end + + def test_sort_natural_with_nested_different_types + input = [ + { "price" => { "value" => 1000, "unit" => "brl" } }, + { "price" => { "value" => 2000, "unit" => "BRL" } }, + { "price" => { "value" => 3000, "unit" => nil } }, + { "price" => { "value" => 4000, "unit" => :brl } } + ] + expected_output = "1000, 2000, 4000, 3000" + template = <<~LIQUID + {{- input | sort_natural: 'price.unit' | map: 'price.value' | join: ', ' -}} + LIQUID + + assert_template_result(expected_output, template, { "input" => input }) + end + + def test_uniq_with_nested_different_types + input = [ + { "price" => { "value" => 1000, "unit" => "BRL" } }, + { "price" => { "value" => 2000, "unit" => "BRL" } }, + { "price" => { "value" => 3000, "unit" => :USD } }, + { "price" => { "value" => 4000, "unit" => :BRL } }, + { "price" => { "value" => 5000, "unit" => nil } } + ] + + expected_output = "BRL, USD, BRL, " # Uniq handles different types uniqueness + template = <<~LIQUID + {{- input | uniq: 'price.unit' | map: 'price.unit' | join: ', ' -}} + LIQUID + + assert_template_result(expected_output, template, { "input" => input }) + end + + def test_map_with_nested_different_types + input = [ + { "price" => { "value" => 1000, "unit" => "brl" } }, + { "price" => { "value" => 2000, "unit" => "BRL" } }, + { "price" => { "value" => 3000, "unit" => nil } }, + { "price" => { "value" => 4000, "unit" => :brl } } + ] + expected_output = "brl, BRL, , brl" + template = <<~LIQUID + {{- input | map: 'price.unit'| join: ', ' -}} + LIQUID + + assert_template_result(expected_output, template, { "input" => input }) + end + + def test_sum_with_nested_different_types + input = [ + { "price" => { "value" => 1000 } }, + { "price" => { "value" => nil } }, + { "price" => { "value" => :none } }, + { "price" => { "value" => 3000 } } + ] + expected_output = "4000" + template = <<~LIQUID + {{- input | sum: 'price.value' -}} + LIQUID + + assert_template_result(expected_output, template, { "input" => input }) + end + private def with_timezone(tz)