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

Attempt accessing only valid item properties #6992

Closed
wants to merge 5 commits into from

Conversation

ashmaroli
Copy link
Member

Resolves #6939

@ashmaroli ashmaroli added the fix label May 6, 2018
@ashmaroli ashmaroli added this to the 4.0 milestone May 6, 2018
@kenman345
Copy link
Contributor

does this need tests with it?

@DirtyF DirtyF requested a review from a team October 28, 2018 05:51
@mattr-
Copy link
Member

mattr- commented Mar 22, 2019

Looks like we need a test for what happens if something isn't an array, at least.

@ashmaroli
Copy link
Member Author

@mattr- Since the time the latest update to where filter was merged in, I hv been trying to see if we can support getting a hash foo's sub-property via use of foo.bar in the where filter..
Will open a PR for that this Sunday and let it close this PR organically..

@mattr-
Copy link
Member

mattr- commented Mar 22, 2019

I'm not ready to introduce the complexity for a foo.bar style sub-property query for the where filter right now this late in the release process. We can consider it for a future release. I'd rather see this PR ship first.

@@ -849,6 +849,20 @@ def to_liquid
assert_equal 2, @filter.where(array, "color", nil).length
end

should "filter array and string objects appropriately" do
Copy link
Member

Choose a reason for hiding this comment

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

Based on this test, the functionality feels too complicated to fix the bug. The most straightforward way I know to test this is to write something like:

should "not throw exception when an item doesn't have the property" do
  array = [{}, { "color" => nil }, { "color" => "" }, { "color" => "text" }, { "foo" => "bar" }]
  assert_equal [{ "color" => "text" }], @filter.where(hash, "color", "text")
end

Also, filtering on sub properties, as in #6939 (context.type), isn't valid syntax. This feels like we need a better reproduction case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Your test-case passes on master. The issue outlined in #6939 is actually due to #item_property encountering a nil element.

Modifying your test-case to the following (will fail on current master:

should "not throw exception when an item doesn't have the property" do
  array = [{}, { "color" => "text" }, { "foo" => "bar" }, nil]
  assert_equal [{ "color" => "text" }], @filter.where(array, "color", "text")
end

Another case that'll fail on master:

should "not throw exception when an item doesn't have the property" do
  array = [{}, { "color" => "text" }, [], { "foo" => "bar" }]
  assert_equal [{ "color" => "text" }], @filter.where(array, "color", "text")
end

@jekyllbot jekyllbot added the has-pull-request Somebody suggested a solution to fix this issue label Jun 5, 2019
@ashmaroli ashmaroli removed this from the 4.0 milestone Jun 24, 2019
@ashmaroli ashmaroli removed the has-pull-request Somebody suggested a solution to fix this issue label Jun 24, 2019
@ashmaroli ashmaroli deleted the item-property-subdata branch February 6, 2020 16:21
@jekyll jekyll locked and limited conversation to collaborators Feb 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Liquid where filter should fail gracefully when property does not exist
5 participants