Supporting optional jsonValueValidator in Operator class?

[budsonjelmont]

In the documentation on JSON rules engine operators it states that the fact passed to the contains/notContains operators must be arrays. And I see in the Operator class that a factValueValidator function can be supplied to the constructor to ensure that the fact supplied to this operator meets that expectation.

Similarly, the documentation states that for in/notIn operators, the value side of the comparison must be an array. But unlike contains/notContains, there's no validation in place to check that this is the case. And that means you don't have the same graceful handling when a non-array value is passed where an array is expected. A minimal-ish example to illustrate how in/notIn are not symmetrical to contains/notContains:

const { Engine } = require('json-rules-engine');

const facts = {
	people: {
		someguy: 'dave',
		otherguys: ['hal', 'stanley', 'alex']
	},
};

// This rule will throw an error because the path $.nonexistentPath evaluates to undefined and b.indexOf(a) throws an error
const operatorInWithNonexistentValuePathRule = {
  conditions: {
    all: [
      {
        fact: 'people',
        path: '$.someguy',
		operator: 'in', // notIn produces the same error
        value: {
          fact: 'people',
		  path: '$.nonexistentpath'
        }
      }
    ]
  },
  event: {
    type: 'in-with-nonexistent-value-path-rule',
  }
};
  
// This rule will NOT throw an error because contains operator is defined with a factValueValidator that will return false if the factValue is not an array
const operatorContainsWithNonexistentFactPathRule = {
	conditions: {
	  all: [
		{
		  fact: 'people',
		  path: '$.nonexistentpath',
		  operator: 'contains',
		  value: {
			fact: 'people',
			path: '$.someguy'
		  }
		}
	  ]
	},
	event: {
	  type: 'contains-with-nonexistent-fact-path-rule',
	}
  };

function runEngine(
	rule,
	facts
){

	const engine = new Engine();
	engine.addRule(rule);

	engine
	.run(facts)
	.then(({ failureEvents }) => {
		failureEvents.map(event => console.log(event));
	})
	.catch(console.error);
}

runEngine(operatorInWithNonexistentValuePathRule, facts);
runEngine(operatorContainsWithNonexistentFactPathRule, facts);

I'm curious if this is the intentional/desired behavior here? Naively I would've expected that in/notIn operators can (and would) validate values similarly to how contains/notContains validate facts. I think this could be accomplished with a minor rewrite to the Operator class, something like:

'use strict'

export default class Operator {
  /**
   * Constructor
   * @param {string}   name - operator identifier
   * @param {function(factValue, jsonValue)} callback - operator evaluation method
   * @param {function}  [factValueValidator] - optional validator for asserting the data type of the fact
   * @param {function}  [jsonValueValidator] - optional validator for asserting the data type of the "value" property of the condition
   * @returns {Operator} - instance
   */
  constructor (name, cb, factValueValidator) {
    this.name = String(name)
    if (!name) throw new Error('Missing operator name')
    if (typeof cb !== 'function') throw new Error('Missing operator callback')
    this.cb = cb
    this.factValueValidator = factValueValidator
    if (!this.factValueValidator) this.factValueValidator = () => true
    this.jsonValueValidator = jsonValueValidator
    if (!this.jsonValueValidator) this.jsonValueValidator = () => true
  }

  /**
   * Takes the fact result and compares it to the condition 'value', using the callback
   * @param   {mixed} factValue - fact result
   * @param   {mixed} jsonValue - "value" property of the condition
   * @returns {Boolean} - whether the values pass the operator test
   */
  evaluate (factValue, jsonValue) {
    return this.factValueValidator(factValue) && this.jsonValueValidator(jsonValue) && this.cb(factValue, jsonValue)
  }
}

And tweaking the initialization of the default engine operators. If there's interest in doing something along these lines I'd be glad to try and throw together a small PR.

[nickmcsimpson]

I want to chime in here as I'm facing a similar issue with my implementation. I noticed in testing that my 'notIn' checks are failing for empty values, when I think they should not be, as described here.

My condition structure:

{
  "operator": "someFact:notIn",
  "value": [
    "123"
  ],
  "fact": "someList",
  "path": "$[*].nestedArray[0].value"
}

If the nestedArray is empty, evaluated path is not found (factValue is undefined in the rule result) meaning that notIn cannot be evaluated. In this example I'm expecting 123 to NOT be in the output collection, so if the output is empty, it should pass, but does not.

Edit:

Digging in further the 'not' decorator gets around my issue with 'notIn'. In my case changing the operator to not:someFact:in behaves as I'm expecting.

[nickmcsimpson]

Following up again as I noticed another situation that's a bit difficult to get around given there's no fallback behavior in the decorators. Here's my secondary use case:

{
  "operator": "swap:everyFact:in",
  "value": [
    "123"
  ],
  "fact": "someList",
  "path": "$[*].nestedArray[0].value"
}

I'm wanting to validate that all the inputs are present in the test data. However, if the given pathway evaluates to an undefined value, then the initial 'sort' causes the issue and the whole test fails out since the error is not being handled internally. The resulting error stems from this line of code (as mentioned by OP):

engine-default-operators.js (line 20)

...
Operators.push(new _operator2.default('in', function (a, b) {
  return b.indexOf(a) > -1;
}));
...

I really think something like the proposed change here would be hugely beneficial. What would we need to get traction on a fix like this?

[budsonjelmont]

Following up again as I noticed another situation that's a bit difficult to get around given there's no fallback behavior in the decorators. Here's my secondary use case:

{
  "operator": "swap:everyFact:in",
  "value": [
    "123"
  ],
  "fact": "someList",
  "path": "$[*].nestedArray[0].value"
}

I'm wanting to validate that all the inputs are present in the test data. However, if the given pathway evaluates to an undefined value, then the initial 'sort' causes the issue and the whole test fails out since the error is not being handled internally. The resulting error stems from this line of code (as mentioned by OP):

engine-default-operators.js (line 20)

...
Operators.push(new _operator2.default('in', function (a, b) {
  return b.indexOf(a) > -1;
}));
...

I really think something like the proposed change here would be hugely beneficial. What would we need to get traction on a fix like this?

@chris-pardy @CacheControl if I took a stab at a PR to implement the suggestion above is there a chance of this being accepted?