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.

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

Nested search in where filter #1436

Closed
wants to merge 39 commits into from

Conversation

andershagbard
Copy link
Contributor

@andershagbard andershagbard commented May 4, 2021

This PR allows nested search in where filter.

{% assign blocks = section.blocks | where: 'settings.some_setting' %}
{% assign blocks = section.blocks | where: 'settings.some_setting', 'blue' %}

Resolves #1388

@andershagbard andershagbard marked this pull request as draft May 4, 2021 10:47
@andershagbard andershagbard marked this pull request as ready for review May 4, 2021 11:57
@henryiii
Copy link

This would be really nice in sort, too. I have a date.start I'd like to sort by, and item.dig(*property.split('.', -1)) would work very well there too I'd think.

Copy link
Contributor

@dylanahsmith dylanahsmith left a comment

Choose a reason for hiding this comment

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

This shouldn't bring any breaking changes to Shopify, but could bring breaking changes to other platforms using Liquid, as it won't consider attributes with dots.

I'm not sure if that is actually the case. Doesn't Shopify support JSON metafield types? If the where filter were used on some parsed json blob, then I would think it could end up filtering on keys with a period in them.

raise_property_error(property)
return [] if ary.empty?
return nil unless ary.first.respond_to?(:[])
return raise_property_error(property) unless ary.first.is_a?(Hash)
Copy link
Contributor

Choose a reason for hiding this comment

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

This would no longer check all the array elements. Also, this seems more restrictive to have it only work for Hash objects rather than supporting any object that responds to [] as the above line implies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added this check:

is_deep = ary.first.is_a?(Hash) && ary.first.include?(property) == false

Is it good enough to only check the first item? I think that checking all items wouldn't perform too well.

Alternatively, we can do something like:

def where(input, property, *target_values = nil)

And then only search nested if target_values contains 2 or more arguments? I just like the dot notation typing more.

lib/liquid/standardfilters.rb Outdated Show resolved Hide resolved
lib/liquid/standardfilters.rb Outdated Show resolved Hide resolved
@andershagbard
Copy link
Contributor Author

This shouldn't bring any breaking changes to Shopify, but could bring breaking changes to other platforms using Liquid, as it won't consider attributes with dots.

I'm not sure if that is actually the case. Doesn't Shopify support JSON metafield types? If the where filter were used on some parsed json blob, then I would think it could end up filtering on keys with a period in them.

In that case, the input would be a string, and this filter shouldn't do anything then?

@dylanahsmith
Copy link
Contributor

I don't mean storing JSON in a string type field, I mean there is native support for JSON metafield types. The primary reason for that json type is so the parsed json is made accessible to liquid, meaning it could be arrays of hashes with keys that could have a period in it.

@andershagbard
Copy link
Contributor Author

I don't mean storing JSON in a string type field, I mean there is native support for JSON metafield types. The primary reason for that json type is so the parsed json is made accessible to liquid, meaning it could be arrays of hashes with keys that could have a period in it.

Got it. Should we cover that case?

@andershagbard
Copy link
Contributor Author

Does it make sense to add this nested feature to other filters, such as sort, uniq etc?

@andershagbard andershagbard changed the title Allow nested search in where filter Nested search in where filter Feb 28, 2022
@andershagbard
Copy link
Contributor Author

Opened a new PR which solves this in a way better way: #1749

@andershagbard andershagbard deleted the where-deep branch January 24, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deeper search in where array filter
3 participants