json-logic-js icon indicating copy to clipboard operation
json-logic-js copied to clipboard

min/max operators don't support vars?

Open NathanHazout opened this issue 6 years ago • 5 comments

In the playground, I tried:

Rule: {"max":{"var": "a"}}

Data: {"a":[1,2,3,4,5]}

Expected Result: 5

Actual Result: null

NathanHazout avatar Mar 13 '18 12:03 NathanHazout

I guess any expression won't work. Similar example of a rule:

{"max": {"if": [true, [1,2,3,4,5], 0] } }

This returns null.

NathanHazout avatar Mar 13 '18 12:03 NathanHazout

@jwadhams I can investigate and try to do a pull request, but I at least want to know if this is expected behavior (why?) or if this is truly a bug.

Thanks

NathanHazout avatar Mar 19 '18 07:03 NathanHazout

Sorry for the slow answer. It's totally a bug, at the intersection of two problems: One, the "single argument syntactic sugar" is applied left-to-right, but results are calculated right-to-left. (That's an oversimplification but it fits your test case)

So as we're evaluating max we see the value of the property is one object and not an array, and we decide you meant [ {"if"...} ] before we recursively evaluate if.

The second problem is that all the variable-length operations (max and + and cat etc) were only written to anticipate variable-number-of-arguments and not one-argument-which-is-an-array.

So a patch that fixes this would probably introduce some helper like

manyArgsOrSingleArray(args){
    if(!args or !args.length){
        return [];
    }else if(args[0] && Array.isArray(args[0])){
        return args[0];
    }else{
        return args;
    }
}

Then update all the places that naively use .call(arguments to instead use .call(manyArgsOrSingleArray(arguments)

And of course we'd need some additional unit tests.

jwadhams avatar Mar 19 '18 14:03 jwadhams

Ok thanks. I am writing a port of json-logic to Go, and I wanted to make sure what the expected behavior is.

NathanHazout avatar Mar 19 '18 15:03 NathanHazout

It's been a weird balancing act of what JsonLogic should do vs what the "main" supported implementations do natively. While I was replying to you, I noticed that this is valid in PHP:

max(1,2,3) // 3
max([1,2,3]) // 3

But only one of these is acceptable JavaScript:

Math.max(1,2,3) // 3
Math.max([1,2,3]) // NaN

This is also why JsonLogic has a separate truthy definition from the implementation languages.

jwadhams avatar Mar 19 '18 15:03 jwadhams