camunda / feel-scala

FEEL parser and interpreter written in Scala
https://camunda.github.io/feel-scala/
Apache License 2.0
120 stars 49 forks source link

is defined() always returns true #695

Closed saig0 closed 10 months ago

saig0 commented 1 year ago

Describe the bug The FEEL engine contains a function is defined(). It can be used to check if a variable or context entry/property exists.

The behavior of this function changed since version 1.17.0.

// === before 1.17.0 ===
is defined("a value")
-> true

is defined(null) 
-> true

is defined(non_existing_variable) 
-> false 

is defined({}.non_existing_key) 
-> false

// === since 1.17.0 ===
is defined("a value")
-> true

is defined(null) 
-> true

is defined(non_existing_variable) 
-> true 

is defined({}.non_existing_key) 
-> true

To Reproduce Steps to reproduce the behavior:

  1. Evaluate the expression is defined(non_existing_variable)
  2. Verify that the evaluation returns true

Expected behavior The function is defined() behaves the same as in version 1.16.0

Environment

saig0 commented 1 year ago

Background

The engine's behavior changed with version 1.17.0 to be more null-friendly. As a result, non-existing variables and context entries result in null instead of failing the evaluation. More generally, the engine returns null for all expressions that can't be applied successfully (i.e. as intended).

Related issues:

saig0 commented 1 year ago

Workaround

The is defined() function can't be used anymore to check if a context contains a given key. To mitigate the issue, we can use the get entries() function with one of the following expressions:

//Check if a context contains a key
"x" in get entries({x:1}).key
-> true

"x" in get entries({x:null}).key
-> true

"y" in get entries({x:1}).key
-> false

// Or using the contains function
list contains(get entries({x:1}).key, "y")
-> false
saig0 commented 1 year ago

Idea

With the new null-friendly behavior in version 1.17.0, it is hard to restore the original behavior of the function.

Instead, we could add a new function to check if a context contains an entry with a given key.

// function signature
context contains(context: context, key: string): boolean

// examples
context contains({x:1}, "x")
-> true

context contains({x:null}, "x")
-> true

context contains({x:1}, "y")
-> false

context contains(null, "y")
-> false

This function could be used together with Zeebe's expression context to check if a variable exists.

context contains(camunda.variables, "x")
saig0 commented 1 year ago

cc: @camunda/zeebe-process-automation @aleksander-dytko

aleksander-dytko commented 1 year ago

@saig0 could you maybe help us with planing by assigning a size to the issue? Thanks!

korthout commented 12 months ago

@aleksander-dytko Philipp is FTO at this time.

ZPA triage (on idea):

saig0 commented 10 months ago

Idea 2

We can't restore the original behavior in version 1.17.0. However, we can mitigate the behavior by changing the semantics of the function slightly.

Instead of checking for a non-existing variable or context entry, we could check for null. Similar to a null-check x != null.

// === before 1.17.0 ===
is defined(null) 
-> true

is defined(non_existing_variable) 
-> false 

is defined({}.non_existing_key) 
-> false

// === since 1.17.0 ===
is defined(null) 
-> false

is defined(non_existing_variable) 
-> false 

is defined({}.non_existing_key) 
-> false

The regression would impact only users if a variable or context entry exists and is null.