Investigate feel-scala 1.17.5 compatibility with camunda-bpm-platform

[psavidis]

Environment (Required on creation)

Camunda 7.21

Description (Required on creation; please attach any relevant screenshots, stacktraces, log files, etc. to the ticket)

The test CustomFunctionTest#shouldThrowExceptionDueToDisabledVarargs fails when the repository tries to be upgraded to use feel-scala:1.17.5.

Steps to reproduce (Required on creation)

• Checkout the master on camunda 7.21.
• Go to the pom.xml of camunda-root and Upgrade the version.feel-scala from 1.6.2 to 1.17.5
• Build the project
• Execute the test CustomFunctionTest#shouldThrowExceptionDueToDisabledVarargs

Observed Behavior (Required on creation)

The test CustomFunctionTest#shouldThrowExceptionDueToDisabledVarargs passes with feel-scala:1.6.2 and fails when the artifact is upgraded to 1.17.5

Expected behavior (Required on creation)

The test should pass

Root Cause (Required on prioritization)

Function registry

The feel engine has a FunctionProvider that will register a function its to internal registry. During evaluation, that registry will be used to check if the function found in the expression the user passed exists.

Enable Varargs Evaluation

The enableVarargs directive or its absence indicate to the engine what parameters to expect during evaluation of the function name

e.g if a function is allowed to be called with variable arguments or not and calls like function(a), function(a,b...) are permitted (or not).

The Test Case

• The test implicitly disables varargs, myFunction can only accept a list.

• The arguments passed are variable arguments

• As a result, the registered function is different from the one passed with varargs. From feel-engine's perspective, the function does not exist.

• The test expects a FeelException to be thrown. The evaluation though returns null

  • There is a breaking change in feel-scala 1.7.0; The invocation of a non-existing function will return null.

Solution Ideas

Adapt the test to expect null instead when upgrading to feel-scala:1.7.15

Hints

Links

Breakdown

### Pull Requests
- [ ] https://github.com/camunda/camunda-bpm-platform/pull/4107

Dev2QA handover

• [ ] Does this ticket need a QA test and the testing goals are not clear from the description? Add a Dev2QA handover comment

[psavidis]

Update

• The CustomFunctionTest.shouldThrowExceptionDueToDisabledVararg is fixed by adjusting the test to expect null responses instead of throwing a FeelException.

There are further test failures that need to be investigated.

Postponing the progress of this item to move forward with other 7.21 items of higher priority.

[psavidis]

Investigation of Other Failures

• FeelBehavior#shouldFailOnInternalContextFunctions

  • Context: The test using feel-scala:1.16.2 throws the following exception: org.camunda.bpm.dmn.feel.impl.juel.FeelMissingFunctionException: FEEL-01007 Unable to resolve function 'dateTime' in expression 'not(dateTime())'
  • Cause: The dateTime function is an internal context function and should not be called. The test tries to use it and the function cannot be found. The response is an exception with missing function in feel-scala:1.6.2
  • Problem: The test fails in feel-scala:1.17.x cause the function returns null instead of throwing an exception
  • Links: ref
  • Solution:
    • The DMN rule should return a tuple containing [null, foo] due to the unique policy of the given associated dmn file.
    • Due to the expectation diverging from JuelFeelBehaviorTest, clone the test from the parent class FeelBehavior to the concrete JuelFeelBehaviorTest & ScalaFeelBehaviorTest and apply the changes only to the ScalaFeelBehaviorTest

• BreakingScalaFeelBehaviorTest#shouldEvaluateTimezoneComparisonWithTypedValue

  • Context: The test using feel-scala:1.16.2 throws the following exception: org.camunda.bpm.dmn.feel.impl.FeelException: FEEL/SCALA-01008 Error while evaluating expression: failed to evaluate expression '<= date and time("2019-09-12T13:00:00@Europe/Berlin")': ValLocalDateTime(2024-02-27T11:01:27.850) can not be compared to ValDateTime(2019-09-12T13:00+02:00[Europe/Berlin])
  • Cause: The following expression should fail during evaluation due to different types being compared:
    • <= date and time("2019-09-12T13:00:00@Europe/Berlin")
  • Problem: The test fails in feel-scala:1.17.x cause the function returns false instead of throwing an exception.
  • Links:
    • #582
  • Solution: Change the test to expect an empty DmnDecisionTableResult

• BreakingScalaFeelBehaviorTest#shouldEvaluateTimezoneComparisonWithDate

  • Same as the above case

• BreakingScalaFeelBehaviorTest#shouldUseSingleQuotesInStringLiterals

  • Context: The test using feel-scala:1.16.2 throws the following exception: org.camunda.bpm.dmn.feel.impl.FeelException: FEEL/SCALA-01008 Error while evaluating expression: failed to parse expression ''Hello World'': Expected (negation | positiveUnaryTests | anyInput):1:1, found \"'Hello Wor\"
  • Cause:
    • Unidentified why the juel expression 'Hello World' expression fails with the message "`Hello Wor"
    • The observation looks as if the closing literal ld' is parsed differently as a reserved word. Couldn't find it under JUEL Reserved Words section
  • Problem: The test fails in feel-scala:1.17.x with an enriched exception message: FEEL/SCALA-01008 Error while evaluating expression: failed to parse expression ''Hello World'': Expected (start-of-input | negation | positiveUnaryTests | anyInput):1:1, found "'Hello Wor".
  • Solution: Change the expected message reason from negation to start-of-input | negation

[psavidis]

Next Step

👀 The CI passed with the adapted tests. Assigning the ticket to @tasso94 for review.

[psavidis]

Update

Incorporated the Code review points and changed the description to reflect accurately the new behaviour of the FEEL engine on comparing different datatypes.

After the final approval on the documentation, the PRs can be merged.

[psavidis]

Update

PRs have been merged to master. Closing the ticket.

[psavidis]

Update

The version in the docs was not correct (1.7 instead of 1.17). Opened a PR to correct it; kudos to @yanavasileva for noticing that 👍