Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression in string conversion of negative duration #599

Closed
korthout opened this issue Mar 8, 2023 · 6 comments · Fixed by #621
Closed

Regression in string conversion of negative duration #599

korthout opened this issue Mar 8, 2023 · 6 comments · Fixed by #621

Comments

@korthout
Copy link
Member

korthout commented Mar 8, 2023

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
Copy link
Member Author

korthout commented Mar 8, 2023

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 added a commit to s-frick/feel-scala that referenced this issue Apr 20, 2023
…meDuration

* fix toString() of ValDayTimeDuration
* add test cases for negative day/time durations
@s-frick
Copy link
Contributor

s-frick commented Apr 20, 2023

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.

@korthout
Copy link
Member Author

korthout commented Apr 21, 2023

@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
Copy link
Contributor

s-frick commented Apr 21, 2023

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

@saig0
Copy link
Member

saig0 commented Apr 26, 2023

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".

@s-frick
Copy link
Contributor

s-frick commented Apr 26, 2023

Okay, I will have a look.

@saig0 saig0 closed this as completed in #621 May 3, 2023
github-actions bot pushed a commit that referenced this issue May 8, 2023
* fix toString() of ValDayTimeDuration
* add test cases for negative day/time durations

(cherry picked from commit 2189ddd)
github-actions bot pushed a commit that referenced this issue May 8, 2023
* fix toString() of ValDayTimeDuration
* add test cases for negative day/time durations

(cherry picked from commit 2189ddd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants