Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add find, find_index, has, and reject filters to arrays #1869

Merged
merged 5 commits into from
Jan 14, 2025
Merged

Add find, find_index, has, and reject filters to arrays #1869

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4b01977
Add reject filter #1573
andershagbard May 12, 2022
1058a77
Add deep search for filter taking in properties #1749
andershagbard Oct 15, 2023
3d1e2d4
Update branch with main
karreiro Dec 3, 2024
41a77c6
Add `find`, `find_index`, `has`, and `reject` filters to arrays
karreiro Dec 3, 2024
2186981
Refactor: avoid usage of public_send
karreiro Dec 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
130 changes: 102 additions & 28 deletions lib/liquid/standardfilters.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ def sort(input, property = nil)
end
elsif ary.all? { |el| el.respond_to?(:[]) }
begin
ary.sort { |a, b| nil_safe_compare(a[property], b[property]) }
ary.sort { |a, b| nil_safe_compare(fetch_property(a, property), fetch_property(b, property)) }
rescue TypeError
raise_property_error(property)
end
Expand Down Expand Up @@ -407,7 +407,7 @@ def sort_natural(input, property = nil)
end
elsif ary.all? { |el| el.respond_to?(:[]) }
begin
ary.sort { |a, b| nil_safe_casecmp(a[property], b[property]) }
ary.sort { |a, b| nil_safe_casecmp(fetch_property(a, property), fetch_property(b, property)) }
rescue TypeError
raise_property_error(property)
end
Expand All @@ -424,29 +424,59 @@ def sort_natural(input, property = nil)
# @liquid_syntax array | where: string, string
# @liquid_return [array[untyped]]
def where(input, property, target_value = nil)
ary = InputIterator.new(input, context)
filter_array(input, property, target_value) { |ary, &block| ary.select(&block) }
end

if ary.empty?
[]
elsif target_value.nil?
ary.select do |item|
item[property]
rescue TypeError
raise_property_error(property)
rescue NoMethodError
return nil unless item.respond_to?(:[])
raise
end
else
ary.select do |item|
item[property] == target_value
rescue TypeError
raise_property_error(property)
rescue NoMethodError
return nil unless item.respond_to?(:[])
raise
end
end
# @liquid_public_docs
# @liquid_type filter
# @liquid_category array
# @liquid_summary
# Filters an array to exclude items with a specific property value.
# @liquid_description
# This requires you to provide both the property name and the associated value.
# @liquid_syntax array | reject: string, string
# @liquid_return [array[untyped]]
def reject(input, property, target_value = nil)
filter_array(input, property, target_value) { |ary, &block| ary.reject(&block) }
end

# @liquid_public_docs
# @liquid_type filter
# @liquid_category array
# @liquid_summary
# Tests if any item in an array has a specific property value.
# @liquid_description
# This requires you to provide both the property name and the associated value.
# @liquid_syntax array | some: string, string
# @liquid_return [boolean]
def has(input, property, target_value = nil)
filter_array(input, property, target_value) { |ary, &block| ary.any?(&block) }
end

# @liquid_public_docs
# @liquid_type filter
# @liquid_category array
# @liquid_summary
# Returns the first item in an array with a specific property value.
# @liquid_description
# This requires you to provide both the property name and the associated value.
# @liquid_syntax array | find: string, string
# @liquid_return [untyped]
def find(input, property, target_value = nil)
filter_array(input, property, target_value) { |ary, &block| ary.find(&block) }
end

# @liquid_public_docs
# @liquid_type filter
# @liquid_category array
# @liquid_summary
# Returns the index of the first item in an array with a specific property value.
# @liquid_description
# This requires you to provide both the property name and the associated value.
karreiro marked this conversation as resolved.
Show resolved Hide resolved
# @liquid_syntax array | find_index: string, string
# @liquid_return [number]
def find_index(input, property, target_value = nil)
filter_array(input, property, target_value) { |ary, &block| ary.find_index(&block) }
end

# @liquid_public_docs
Expand All @@ -465,7 +495,7 @@ def uniq(input, property = nil)
[]
else
ary.uniq do |item|
item[property]
fetch_property(item, property)
rescue TypeError
raise_property_error(property)
rescue NoMethodError
Expand Down Expand Up @@ -501,7 +531,7 @@ def map(input, property)
if property == "to_liquid"
e
elsif e.respond_to?(:[])
r = e[property]
r = fetch_property(e, property)
r.is_a?(Proc) ? r.call : r
end
end
Expand All @@ -525,7 +555,7 @@ def compact(input, property = nil)
[]
else
ary.reject do |item|
item[property].nil?
fetch_property(item, property).nil?
rescue TypeError
raise_property_error(property)
rescue NoMethodError
Expand Down Expand Up @@ -899,7 +929,7 @@ def sum(input, property = nil)
if property.nil?
item
elsif item.respond_to?(:[])
item[property]
fetch_property(item, property)
else
0
end
Expand All @@ -918,6 +948,50 @@ def sum(input, property = nil)

attr_reader :context

def filter_array(input, property, target_value, &block)
ary = InputIterator.new(input, context)

return [] if ary.empty?

block.call(ary) do |item|
if target_value.nil?
fetch_property(item, property)
else
fetch_property(item, property) == target_value
end
rescue TypeError
raise_property_error(property)
rescue NoMethodError
return nil unless item.respond_to?(:[])
raise
end
end

def fetch_property(drop, property_or_keys)
karreiro marked this conversation as resolved.
Show resolved Hide resolved
karreiro marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

@ianks ianks Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how I feel about re-implementing lookup logic here. There's already a way to do that which is well-defined with VariableLookup.parse. It even has built-in caching to mitigate performance issues of repeated parsing. Could we use this instead?

irb(main):006> Liquid::VariableLookup.parse("product.images[0]")
=> #<Liquid::VariableLookup:0x000000010565ce98 @command_flags=0, @lookups=["images", 0], @name="product">

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reservations about parsing strings at render time in general. At the very least it will play havoc with static analysis.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's continue this thread here:
#1899 :)

##
# This keeps backward compatibility by supporting properties containing
# dots. This is valid in Liquid syntax and used in some runtimes, such as
# Shopify with metafields.
#
# Using this approach, properties like 'price.value' can be accessed in
# both of the following examples:
#
# ```
# [
# { 'name' => 'Item 1', 'price.price' => 40000 },
# { 'name' => 'Item 2', 'price' => { 'value' => 39900 } }
# ]
# ```
value = drop[property_or_keys]

return value if !value.nil? || !property_or_keys.is_a?(String)

keys = property_or_keys.split('.')
keys.reduce(drop) do |drop, key|
drop.respond_to?(:[]) ? drop[key] : drop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems weird to me... does this mean that:

product.dafsdfasdfasdf #=> ProductDrop

I would have expected it to be like:

product.dafsdfasdfasdf #=> nil

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be weird indeed. But I believe it's already working as expected (and product.dafsdfasdfasdf already returns nil), as this test passes:

def test_prop_test
  products = [
    { "title" => "Pro goggles" },
    { "title" => "Thermal gloves", "price" => 1499 },
    { "title" => "Alpine jacket",  "price" => 3999 },
    { "title" => "Mountain boots", "price" => nil },
    { "title" => "Safety helmet",  "price" => 1999 },
  ]

  template = <<~LIQUID
    {%- assign example1 = products | find_index: 'dafsdfasdfasdf' -%}
    {{- example1 -}}
    {%- assign example2 = products | find_index: 'dafsdfasdfasdf.dafsdfasdfasdf' -%}
    {{- example2 -}}
  LIQUID
  expected_output = ""

  assert_template_result(expected_output, template, { "products" => products })
end

The reason we're achieving the expected behavior is that:

  • For example1, we're resolving that at line 985
  • For example2, we're resolving that because in the first iteration drop is product and returns nil for the first dafsdfasdfasdf call, then in the second call drop is already nil

So, I believe we're already achieving that behavior, but please let me know if I'm missing something :)

end
charlespwd marked this conversation as resolved.
Show resolved Hide resolved
end

def raise_property_error(property)
raise Liquid::ArgumentError, "cannot select the property '#{property}'"
end
Expand Down
Loading
Loading