camunda/feel-scala

Regression in string conversion of negative duration

korthout opened this issue · 6 comments

korthout commented

Describe the bug
A negative duration is converted to a positive duration (double negation).

To Reproduce

===== FEEL Engine REPL (1.15.3) ======
> Evaluate FEEL expressions using 'feel("1 + 3")'
> Evaluate FEEL unary-tests using 'unaryTests("[2..5]", 4)'
> Provide variables as Map using 'feel("x + 3", Map("x" -> 2))'
> Provide variables as JSON string using 'feel("x + 3", "{ \"x\" : 2 }")'
Welcome to the Ammonite Repl 2.5.6 (Scala 3.2.1 Java 19.0.2)
@ feel("string(duration(\"-PT1S\"))")
> -P-1S

===== FEEL Engine REPL (1.15.2) ======
> Evaluate FEEL expressions using 'feel("1 + 3")'
> Evaluate FEEL unary-tests using 'unaryTests("[2..5]", 4)'
> Provide variables as Map using 'feel("x + 3", Map("x" -> 2))'
> Provide variables as JSON string using 'feel("x + 3", "{ \"x\" : 2 }")'
Welcome to the Ammonite Repl 2.5.6 (Scala 3.2.1 Java 19.0.2)
@ feel("string(duration(\"-PT1S\"))")
> PT-1S

Expected behavior
The result of string(duration("-PT1S")) should be "-PT1S"

Environment

  • FEEL engine version: 1.15.3
  • Affects:
    • Camunda Platform 8: >8.1.7
korthout commented

We (Zeebe Process Automation) want to patch this, but wrapping up the Zeebe 8.2 release has our priority. Marking this as later so we may work on it after the 8.2 release.

s-frick commented

I've searched for an Issue for my first contribution on open source and gave this one a try. I found a solution to fix this. But i have a question related to the expected behavior. The underlying java.time.Duration parses from an expression like "-PT1M1S" an equivalent expression "PT-1M-1S". If this fits your needs I'm done. If not then there is a little more work to do. I will open a pull request and love to see feedback.

  • 🎉1
korthout commented

@sebi87 That's great! I see that @saig0 is already assigned as reviewer 🎉

I just noticed that I made a typo in the expected behavior:

The result of string(duration("-PT1S")) should be "PT1S"

Should of course be:

The result of string(duration("-PT1S")) should be "-PT1S"

It should also be fine if this becomes "PT-1S" as this is equivalent.

s-frick commented

@korthout yeah, I assumed that it’s a typo

  • 👍1
saig0 commented

It should also be fine if this becomes "PT-1S" as this is equivalent.

The DMN spec is not very clear about the format:

image

(DMN 1.4, page 108)

The reference to the XPath document is not much better:

image

However, I found a related test case in the DMN TCK. Since this is the reference for all DMN vendors, we should follow this example to pass the test case.

image

Based on this test case, the format should be "-P1D".

  • 👍1
s-frick commented

Okay, I will have a look.

  • 👍1