Where filter does not handle truthy values correctly

[williamb1024]

When using the where filter with a single argument, the filter does not perform truthy comparisons. The equality comparison is performed with the property value controlling.

https://github.com/sebastienros/fluid/blob/74cb7421a6400acb425fd7448552475f2f18178c/Fluid/Filters/ArrayFilters.cs#L156

Switching itemValue and targetValue, such that targetValue is controlling would result in truthy comparisons.

[MoazAlkharfan]

Here's a workaround with adding a new filter named twhere

Usage twhere instead of where:

{% assign filteredItems = items | twhere: "property" %}

Register in the filters:

builder.AddFluid(x => {
    x.TemplateOptions.Filters.AddFilter(WhereFilter.Name, WhereFilter.Where);
});

Filter implementation with workaround:

internal static class WhereFilter
{
    internal const string Name = "twhere";

    // Track issue: https://github.com/sebastienros/fluid/issues/620
    internal static async ValueTask<FluidValue> Where(FluidValue input, FilterArguments arguments, TemplateContext context)
    {
        if (input.Type != FluidValues.Array)
        {
            return input;
        }

        // First argument is the property name to match
        var member = arguments.At(0).ToStringValue();

        // Second argument is the value to match, or 'true' if none is defined
        var targetValue = arguments.At(1).Or(BooleanValue.True);

        var list = new List<FluidValue>();

        foreach (var item in input.Enumerate(context))
        {
            var itemValue = await item.GetValueAsync(member, context);

            if (itemValue.Equals(targetValue) ||
                // Issue workaround for equality inconsistencies: to check if the other equals self
                targetValue.Equals(itemValue))
            {
                list.Add(item);
            }
        }

        return new ArrayValue(list);
    }
}

[sebastienros]

So instead of comparing to true we actually need to handle the single argument separately and convert it using ToBooleanValue(). Can someone create a PR and a test for that?

[gabbersepp]

@MoazAlkharfan I don't think that this is a correct fix. According to the shopify liquid documentation, everything is truthy, except nil and false which also means, that I easily can use:

where: "Name" to find all items where the property Name has any value.

Am I right?

@sebastienros What do you think?

[williamb1024]

The solution I came up with is:

public static async ValueTask<FluidValue> WhereFilter(FluidValue input, FilterArguments filterArguments, TemplateContext templateContext)
        {
            // this filter corrects an issue with the builtin where filter that causes it not to
            // support the documented "truthy" comparisons if no second argument is used.

            if (input.Type != FluidValues.Array)
                return input;

            var propertyName = filterArguments.At(0).ToStringValue();
            var targetValue = filterArguments.At(1).Or(BooleanValue.True);

            var list = new List<FluidValue>();

            foreach (var item in input.Enumerate(templateContext))
            {
                var itemValue = await item.GetValueAsync(propertyName, templateContext);

                // compare with targetValue on the left. This will force the comparison to coerce the
                // itemValue to the targetValue type, rather than the other way around. This makes "truthy" 
                // comparisons work properly.

                if (targetValue.Equals(itemValue))
                    list.Add(item);
            }

            return new ArrayValue(list);
        }

Based on my reading of the Liquid documentation, that implementation met all of the requirements. If it is correct, I will create a PR.

[sebastienros]

As I mentioned, and as @gabbersepp is also noting, .Or(BooleanValue.True) would mean that where: "name" would only work if name is actually the value true too.

I think instead if there is no tagetValue it should check if GetValueAsync(propertyName).ToBooleanValue() is true

[williamb1024]

Removing the Or and checking targetValue for nil and performing a different operation based on that condition would, I believe, produce the same results.

I believe my code is a better choice in that is ultimately a much smaller code change.

https://github.com/sebastienros/fluid/blob/30829ad673e21826c63c0de9da7149b5ab92749a/Fluid/Filters/ArrayFilters.cs#L133-L159

As you can see, the only real change is:

                // compare with targetValue on the left. This will force the comparison to coerce the
                // itemValue to the targetValue type, rather than the other way around. This makes "truthy" 
                // comparisons work properly.

                if (targetValue.Equals(itemValue))
                    list.Add(item);

[sebastienros]

Please create a PR then, we can add some tests to verify all requirements.

[gabbersepp]

Sry, maybe I don't understand the code. But where is the part that checks if member contains any string value if no second parameter is specified?

[sebastienros]

@gabbersepp I agree that this won't work, but I am ok with @williamb1024 create the initial PR so we can add a unit test to show what's missing, and then we can simply add my recommendation (that's my evil plan)

[williamb1024]

@gabbersepp

When the second parameter is undefined, targetValue is assigned the value BooleanValue.True.

When the targetValue.Equals(itemValue) is performed and targetValue is BooleanValue.True, the result is true if itemValue is "truthy".

In liquid, any value that is not nil or false is "truthy".

@sebastienros I'm excited to see the results of your unit tests.

[sebastienros]

Thanks @williamb1024 , feel free to send more!