jwadhams / json-logic-js

Build complex rules, serialize them as JSON, and execute them in JavaScript
MIT License
1.27k stars 139 forks source link

Consistent, language independent logic #44

Open YaraslauZhylko opened 6 years ago

YaraslauZhylko commented 6 years ago

I was trying to find a way to store simple automation rules in the database and pass them between UI and backend. And json-logic seems to be a perfect fit for that purpose.

So I took the Python port of it and found out it was not working as expected in some cases. Trying to mend the port by creating a pull request I referred to this repo to get the etalon behaviour.

But I soon found out that some operators (only numeric operations so far) behave inconsistently or are dependent on JavaScript logic. While they should probably be following human (or Vulcan :) ) logic to make json-logic language-independent.

Here are some examples that I've tested using the "Play with It" page:

Arithmetic:

Comparison:

Min/max:

P.S.: Comments above assume that all null values must be treated as bad values for arithmetic. But if users want to treat null as 0 or 1 they may always use the default value argument for "var".

I think that code for those operators should be reviewed to ensure etalon, language/platform-independent behaviour of json-logic.

What's your opinion, @jwadhams?

YaraslauZhylko commented 6 years ago

I also suppose that the following operators should be made chainable too (with minor modifications regarding number of arguments and null values):

YaraslauZhylko commented 6 years ago

Alternatively, all comparison operators could be forced to take exactly 2 arguments and evaluation operators - exactly 1 argument. Throwing exceptions if there number is incorrect.

But in that case we'll probably have to dump the "between" functionality and replace it with "and":

YaraslauZhylko commented 6 years ago

Or we could spare the "<" and "<=" operators allowing them to have a third optional argument.

But that would sacrifice consistency...

YaraslauZhylko commented 6 years ago

@jwadhams,

I've already forked the repository and made changes described in the first two posts on my local machine. I'm ready to commit them and initiate a pull request as soon as you decide whether those changes are viable for the project.

I'm also ready to update the Python repository to match the latest JS version if needed.

YaraslauZhylko commented 6 years ago

A small example of why I consider nulls should be ruled out:

Let's imagine a simple thermostat that reads room temperature and turns the heating on when it's cold.

It has the following rule:

{"if": [
    {"<": [{"var": "room_temperature"}, 15]}, "We are freezing! Turn all heating on!",
    "Keep calm and carry on."
]}

If it gets normal temperature readings it does nothing:

{"room_temperature": 23.5} => "Keep calm and carry on."

If the temperature gets a little colder the thermostat turns the heating on:

{"room_temperature": 9.5} => "We are freezing! Turn all heating on!"

But what is the data is missing? The sensor gets damaged or disconnected? Connection drops? Or there's just no data yet?

{"room_temperature": null} => "We are freezing! Turn all heating on!"
{} => "We are freezing! Turn all heating on!"

That's what we'll get!

Json Logic decides that it's time to panic and switch everything on just because we do not have a value to analyse. The rule is all about analysing a valid value. It cannot make decisions without it. But it actually does. Because it assumes null to be 0. And 0 < 15.

How can we tell if the apple is tasty when we've got none? Let's just assume it is tasty without actually tasting it? :) The rule should fail stating that the apple IS NOT tasty. Want to check if it is sour? Add another rule. But it should also fail stating that the apple IS NOT sour either. Every assumption should fail if we got no valid data.

We can write a correct rule to check for value existence. But in current Json Logic implementation we'll have to overcomplicate it:

{"if": [
    {"and": [
        {"!": [{"missing": "room_temperature"}]},
        {"<": [{"var": "room_temperature"}, 15]}
    ]}, "We are freezing! Turn all heating on!",
    "Keep calm and carry on."
]}

Looks ugly to me...

And even more ugly if actual values are retrieved on demand or require some other form of extensive calculation. Because under the hood the "missing" operator actually retrieves the value to make sure it is not null or empty string.

But if we rule nulls out of the evaluation the original "pretty" rule will return the expected result according to the human logic.

{"room_temperature": 23.5} => "Keep calm and carry on."
{"room_temperature": 9.5} => "We are freezing! Turn all heating on!"
{"room_temperature": null} => "Keep calm and carry on."
{} => "Keep calm and carry on."

Even if we add another "else if" statement...

{"if": [
    {"<": [{"var": "room_temperature"}, 15]}, "We are freezing! Turn all heating on!",
    {">=": [{"var": "room_temperature"}, 15]}, "I'm comfortable with that...",
    "Keep calm and carry on."
]}

...the results will still be acceptable for human logic as none of the conditions will apply:

{"room_temperature": 23.5} => "I'm comfortable with that..."
{"room_temperature": 9.5} => "We are freezing! Turn all heating on!"
{"room_temperature": null} => "Keep calm and carry on."
{} => "Keep calm and carry on."

But what about the guys and gals who still want nulls to be treated as 0's? There desires will be much easer to implement compared to using the "missing" operator:

{"var": ["room_temperature", 0]}

That's all folks! :)

You just explicitly state: "If there's no value use 0 instead." And from now on there's no more assumptions as to how the empty value is treated.

P.S.: Implicit assumptions get even worse when it comes to arithmetic where "+", "-", "*" and "/" operators treat nulls differently...

P.P.S.: Nothing to say about different implicit assumptions in different programming languages. :) And Json Logic is supposed to act precisely the same in all of them to be efficient.

josephdpurcell commented 1 year ago

This is excellent. Has anyone considered making a semi-formal (or formal) specification for JSON logic? I don't see something resembling a "spec", I looked at https://jessemitchell.me/json-logic-engine/ too.

Edit: I just saw that there is https://jsonlogic.com/tests.json, maybe that is the "spec"?