instance of doesn't work for durations

[saig0]

Describe the bug I can not check the type of a duration value using instance of.

To Reproduce Expression null instance of years and months duration produces a failure but should return false

Expected behavior

null instance of years and months duration -> false
null instance of days and time duration -> false

duration("P3M") instance of years and months duration -> true
duration("PT4H") instance of days and time duration -> true

Environment

• FEEL engine version: 1.11.2

[daniel-shuy]

Debugging the interpreter, it seems that the interpreter is expecting:

duration("P3M") instance of year-month-duration
duration("PT4H") instance of day-time-duration

However, the - symbol is being interpreted as a Subtraction operation, which is causing the error. This would require enhancing the interpreter to ignore numeric operators if they are not following or followed by a number, which will be very difficult to do.

Just changing the expected type to years and months duration/days and time duration is not a simple feat either, as the instance of function only expects 1 word after it.

Also, the instance of function currently returns true for nulls (regardless of the type), which is the correct behavior.

[saig0]

@daniel-shuy thank you for having a look :+1:

Just changing the expected type to years and months duration/days and time duration is not a simple feat either, as the instance of function only expects 1 word after it.

Yes. We need to tweak the parser a bit to accept "years and months duration" and "days and time duration" as type names. See here: https://github.com/camunda/feel-scala/blob/master/src/main/scala/org/camunda/feel/impl/parser/FeelParser.scala#L357-L363

Also, the instance of function currently returns true for nulls (regardless of the type), which is the correct behavior.

I guess that you mean that it returns false if the value is null. Yes, this is the correct behavior.

[sccalabr]

id like to give this a try.

[saig0]

@sccalabr yes, go for it. :rocket:

Don't forget to

• write new test cases here
• update the docs here

[sccalabr]

Is this link still the correct link https://github.com/camunda/feel-scala/blob/master/src/main/scala/org/camunda/feel/impl/parser/FeelParser.scala#L357-L363

[saig0]

@sccalabr no, you're right. :+1:

In the parser, we would need to extend the function valueProperty() and add the new property names. See here.

[sccalabr]

Is there more that needs to be done because when I do that I get

• should instance of durations *** FAILED *** ValError(failed to parse expression ' duration("P3M") instance of years and months duration': Expected (binaryComparison | between | instanceOf | in | "and" | "or" | end-of-input):1:47, found "duration") was not equal to ValBoolean(true) (DateTimeDurationPropertiesTest.scala:344) Analysis: ValError(error: failed to parse expression ' duration("P3M") instance of years and months duration': Expected (binaryComparison | between | instanceOf | in | "and" | "or" | end-of-input):1:47, found "duration" -> , value: -> true)

https://github.com/camunda/feel-scala/compare/main...sccalabr:feel-scala:155?expand=1

[saig0]

@sccalabr I see. Let me revert my previous comment. :sweat_smile: Instead, we need to adjust the function typeName() that is used by the instanceOf() function in the parser.

[sccalabr]

adjust the function how? the names map to identifier | escapedIdentifier do we consider those strings to be reserved words now?

[saig0]

@sccalabr you could extend the typeName() function and add the new two duration type names. Since the duration type names contain whitespace, the type names are not parsed by qualifiedName.

For example:

private def typeName[_: P]: P[String] =
    P(
      durationTypeName |
      qualifiedName.map(_.mkString("."))
    )

private def durationTypeName[_: P]: P[String] =
    P(
      "years and months duration" |
      "days and time duration" 
    ).!

The types are no reserved words but need to be handled explicitly because of the whitespaces.

[sccalabr]

How would instance of know that duration is of those types though? I would have thought the exp class would have made it work but that doesnt seem to be the case.

Here is the pr I have so its easier to see https://github.com/camunda/feel-scala/pull/535

[saig0]

@sccalabr the PR looks good so far. :+1: Except for a typo in ears and months duration and that the function arguments must be wrapped into double quotes (i.e. duration("P3M") vs. duration(P3M)).

In order to pass the test, you need to adjust the function withType in FeelInterpreter.