feat: Non-existing context entries result in null
#696
Conversation
|
@korthout please review the PR in the next 4 weeks (or assign to someone else). 😉 |
There was a problem hiding this comment.
Nice changes @saig0 🚀
👍 I especially like how the tests are becoming more expressive
🔧 I have just a few comments, but nothing blocking. Please have a look.
👍 LGTM
💭 As this behavior breaks user space (and regresses #695), there must be clear communication to users in the release notes about these changes. Perhaps this requires a major version bump. WDYT?
PS: Sorry for the late review
src/test/scala/org/camunda/feel/impl/interpreter/InterpreterContextExpressionTest.scala
Show resolved
Hide resolved
src/main/scala/org/camunda/feel/impl/interpreter/FeelInterpreter.scala
Outdated
Show resolved
Hide resolved
| error( | ||
| failureType = EvaluationFailureType.NO_CONTEXT_ENTRY_FOUND, | ||
| failureMessage = s"context contains no entry with key '$key'" | ||
| failureMessage = s"No context entry found with key '$key'. $detailedMessage" | ||
| ) | ||
| ValNull |
There was a problem hiding this comment.
❓ Off-topic: I was wondering a bit about the usage of the implicit EvalContext and the return value of the error() function. The return value is now ignored because ValNull is returned instead. this works, but the error function is now used only for its 'side effect' (i.e. storing the error as a failure in the implicit evaluation context). It feels easy to use this way, but is this idiomatic Scala? Because this is a side-effect right?
There was a problem hiding this comment.
Valid point. 😅 This style is only temporary. After the remaining null-handling issues are resolved, I'll clean up the error part.
src/test/scala/org/camunda/feel/impl/interpreter/InterpreterContextExpressionTest.scala
Outdated
Show resolved
Hide resolved
Add a test case to verify that the evaluation returns null if a context entry doesn't exist.
Change the behavior to return null if a context doesn't contain an entry with the given key, or if the context is null. Previously, the evaluation failed here.
Add a test case to verify that a context projection contains null if a context entry doesn't have the given key. Group the new test case with the other context projection test cases.
Group all test cases related to context path expressions.
Group all test cases related to context filter expressions.
Use the new evaluation result matcher in all test cases to improve the readability and to align on one format.
The behavior for bean expressions (i.e. a special form of contexts) changed. If a field or method is not available/visible, the expression results in `null`. Previously, the evaluation failed. Use the new base test class and the result matcher to improve the readability of the test cases.
The test case for the `is defined()` function doesn't work anymore because with the new behavior a non-existing context entry returns null. As a result, the function invocation returns `true` instead of `false`. There is an issue to deal with the changed behavior of the function. See more here: #695. Ignoring the test case until the issue is solved.
A non-existing context entry results in `null`. Rewrite the test case to keep the expectation that the evaluation fails.
The failure message for a non-existing context entry changed. Adjust the expected message of the test case.
Wrap available keys and properties inside single quotes to improve the readability of the message.
saig0
force-pushed
the
674-null-friendly-context
branch
from
0ac3ac2
to
66e72f8
Compare
I agree that we must be very clear with the communication and provide solutions. 👍 I don't see the need for a major version here. From the spec perspective, the current behavior is a bug and we're fixing the bug. In most cases, the impact on the user should be minor because the However, a major version would not help in the context of Camunda 8. I don't think that it would change something about the integration of the FEEL engine. |
Description
nullif a context doesn't contain an entry with the given key, or if the context is null. Previously, the evaluation failed in this case.Related issues
contributes to #674