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

Unable to apply filter expression to a list literal #25

Closed PaulChernoch-Shell closed 3 years ago

PaulChernoch-Shell commented 3 years ago

If a list is part of the context, you may use a filter expression on it. However, if the list is a list literal, attempting to extract data from it using a filter expression produces a parse error. A corresponding problem exists if you try to access a property of a context literal using the dot operator.

I have verified that the failing expression parses and properly exedcutes against a Feel 1.3 implementation. I assume but am not sure that 1.1 was supposed to support this case.

SUCCESSFUL PARSE:

Parse tree for rule 'employees[3]' is:

{
  "type": "Program",
  "body": {
    "type": "FilterExpression",
    "expr": {
      "type": "Name",
      "nameChars": "employees",
      "loc": {"start": {"offset": 0,"line": 1,"column": 1},"end": {"offset": 9,"line": 1,"column": 10}},
      "text": "employees"
    },
    "filterExpr": {
      "type": "Literal",
      "value": 3,
      "loc": {"start": {"offset": 10,"line": 1,"column": 11},"end": {"offset": 11,"line": 1,"column": 12}},
      "text": "3"
    },
    "loc": {"start": {"offset": 0,"line": 1,"column": 1},"end": {"offset": 12,"line": 1,"column": 13}},
    "text": "employees[3]"
  },
  "loc": {"start": {"offset": 0,"line": 1,"column": 1},"end": {"offset": 12,"line": 1,"column": 13}
  },
  "text": "employees[3]"
}

UNSUCCESSFUL PARSE:

Parse tree for rule '["Sally", "Harry", "Dilbert", "Dogbert"] [3]' is:

peg$SyntaxError: Expected end of input or whitespace but "[" found.
    at peg$buildStructuredError (c:\Users\pchernoch\projects\feel\dist\feel.js:690:12)
    at Object.peg$parse [as parse] (c:\Users\pchernoch\projects\feel\dist\feel.js:8089:11)
    at test_parse (c:\Users\pchernoch\projects\feel-test\test-feel-expression.js:18:24)
    at main (c:\Users\pchernoch\projects\feel-test\test-feel-expression.js:182:21)
    at Object.<anonymous> (c:\Users\pchernoch\projects\feel-test\test-feel-expression.js:185:1)
    at Module._compile (internal/modules/cjs/loader.js:1063:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1092:10)
    at Module.load (internal/modules/cjs/loader.js:928:32)
    at Function.Module._load (internal/modules/cjs/loader.js:769:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12) {
  expected: [ { type: 'other', description: 'whitespace' }, { type: 'end' } ],
  found: '[',
  location: {
    start: { offset: 41, line: 1, column: 42 },
    end: { offset: 42, line: 1, column: 43 }
  }
}
PaulChernoch-Shell commented 3 years ago

The above issue seems to spring from this parser grammar rule:

FilterExpression
    = head:LeftExph __ "[" __ tail:Expression __ "]"
        {
            log(`FilterExpression (${text()})`);
            return new ast.FilterExpressionNode(head,tail,location(), text(), rule());
        }

In the specification for DMN 1.2, p113 of section 2.3.1.2, the rule is:

52. filter expression = expression , "[" , expression , "]" ;

Use of head:LeftExph restricts the LHS to TxtExpi, which is Name, Literal or SimplePositiveUnaryTest.

I suspect there is a recursion problem in the grammar that this rule hopes to avoid, but it eliminates valid expressions from being parsed. I will see if I can tweak the grammar on my machine to fix it.

PaulChernoch-Shell commented 3 years ago

I have found a fix. In the grammar, make these changes:

  1. Modify the Expression rule by adding a Negative lookahead to prevent promoting a BoxedExpression to Expression if it is followed by a selector or a path operator.
Expression
    = expr:BoxedExpression !(__ ("[" / "."))
        {
            return expr;
        }
    / TextualExpression
  1. Modify the FilterExpression rule by adding an alternative that accepts a BoxedExpression.
FilterExpression
    = head:LeftExph __ "[" __ tail:Expression __ "]"
        {
            log(`FilterExpression (${text()})`);
            return new ast.FilterExpressionNode(head,tail,location(), text(), rule());
        }
    / head:BoxedExpression __ "[" __ tail:Expression __ "]"
        {
            log(`FilterExpression (${text()})`);
            return new ast.FilterExpressionNode(head,tail,location(), text(), rule());
        }
  1. Modify the PathExpression rule by adding an alternative that accepts a BoxedExpression.
PathExpression
    = head:LeftExpg tail: (__ "." __ Expression)+
        {
            log(`PathExpression (${text()})`);
            return new ast.PathExpressionNode(buildList(head,tail,3),location(), text(), rule());
        }
    / head:BoxedExpression tail: (__ "." __ Expression)+
        {
            log(`PathExpression (${text()})`);
            return new ast.PathExpressionNode(buildList(head,tail,3),location(), text(), rule());
        }

It would probably be a good idea to add some regression tests. I verified that this executes correctly for some simple cases.

deostroll commented 3 years ago

Hi. I have made the changes. It doesn't seem to disrupt the other tests. It will be impossible for me to discover these issues, much less come out with a fix such as this. So thanks again.

The changes will come into the repo in the following week.