EdgeVerve / feel

Expression Language for creating and executing business rules in decision table based on DMN 1.1 specification for conformance level 3
MIT License
93 stars 48 forks source link

date("2012-12-23") >= date("2012-12-24") evaluates to true #10

Closed a-hegerath closed 3 years ago

a-hegerath commented 6 years ago

The comparison of dates (and probably also times) seems to be wrong for inequalities (>, >=, <, <=).

This can be proved by adding the following test case to the file ./test/comparision-expression/feel-comparision-expression.build.spec.js

  it('Successfully compare dates with ">="', function(done) {
    var text = 'date("2012-12-23") >= date("2012-12-24")';
    // another error case: var text = 'date("2011-12-25") > date("2012-12-24")';
    var parsedGrammar = FEEL.parse(text);
    parsedGrammar.build().then(result => {
      expect(result).to.be.false;
      done();
    }).catch(err => done(err));
  });

The error is likely caused by the function "checkInequality" in file utils/helper/fn-generator.js, which applies the operator component-wise (with "component", I mean the date/time parts like year, month, day and so on), which works for equalities but not for inequalities. A possible solution would be to compare the components using two operators - by this we can decide for each component if the inequality holds, or if it does not hold, or if we cannot say and have to proceed to the next component.

function checkInequality(opTrue, opFalse) {
  const fnTrue = operatorMap[opTrue]; // the operator to decide if the inequality holds
  const fnFalse = operatorMap[opFalse]; // the reverse operator to decide if the inequality does not hold
  return function (x, y, props) {
    return props.reduce((recur, next) => {
      if (recur !== 0) { // we already know the result from a previous component
        return recur;
      }
      if (fnTrue(x[next], y[next])) {
        // now we know that the inequality holds
        return 1;
      }
      if (fnFalse(x[next], y[next])) {
        // now we know that the inequality does not hold
        return -1;
      }
      // we still do not know and have to proceed to the next component
      return 0;
    }, 0);
  };
}

Usage of the checkInequality function:

// for <
  const checkLt = checkInequality('<', '>'); // eslint-disable-line no-use-before-define
  return checkLt(x, y, dateTimeComponent.date) > 0;
// for <=
  const checkLt = checkInequality('<', '>'); // eslint-disable-line no-use-before-define
  return checkLt(x, y, dateTimeComponent.date) >= 0; // eslint-disable-line no-use-before-define
// for >
  const checkGt = checkInequality('>', '<'); // eslint-disable-line no-use-before-define
  return checkGt(x, y, dateTimeComponent.date) > 0;
// for >=
  const checkGt = checkInequality('>', '<'); // eslint-disable-line no-use-before-define
  return checkGt(x, y, dateTimeComponent.date) >= 0; // eslint-disable-line no-use-before-define
deostroll commented 6 years ago

Hi, Thanks for the suggestion. I have fixed this in a simpler way. We only can roll out a fix next release. However, if you want to get your hands dirty you can try my fork with the specific changes. (Note: other changes are there too).

https://github.com/deostroll/feel/tree/a0ee54b78fa6751ea38c24c0c3eaa8ee619062dc

Let me know if this works for you. It fixed issues 10/11 I guess.