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

is defined() always returns true #695

Closed
saig0 opened this issue Jul 27, 2023 · 7 comments · Fixed by #712
Closed

is defined() always returns true #695

saig0 opened this issue Jul 27, 2023 · 7 comments · Fixed by #712
Assignees
Labels
scope: Camunda 8 Required in the context of Camunda 8 target:8.3 Planned for the Camunda 8.3 release type: bug

Comments

@saig0
Copy link
Member

saig0 commented Jul 27, 2023

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

  • FEEL engine version: 1.17.0 (not yet released - on main branch)
  • Affects:
    • Camunda Automation Platform 7: [7.x]
    • Zeebe broker: [0.x]
@saig0 saig0 added type: bug scope: Camunda 8 Required in the context of Camunda 8 labels Jul 27, 2023
@saig0
Copy link
Member Author

saig0 commented Jul 27, 2023

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

saig0 commented Jul 27, 2023

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

saig0 commented Jul 27, 2023

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

saig0 commented Jul 27, 2023

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

saig0 added a commit that referenced this issue Jul 28, 2023
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.
@saig0 saig0 added the target:8.3 Planned for the Camunda 8.3 release label Jul 28, 2023
@aleksander-dytko
Copy link

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

@korthout
Copy link
Member

korthout commented Aug 2, 2023

@aleksander-dytko Philipp is FTO at this time.

ZPA triage (on idea):

  • size: medium (team is still unfamiliar with feel-scala codebase, but we think we can resolve this)
  • however we think the regression would not be resolved by this idea
  • we'll favor other feel-scala issues because we don't know how to resolve the regression
  • marking as later

saig0 added a commit that referenced this issue Sep 5, 2023
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.
saig0 added a commit that referenced this issue Sep 6, 2023
The built-in function `is defined()` doesn't work anymore. By returning `null` for a non-existing variable, the function returns always `true`. Previously, the function was used to check if a variable or context entry exists.

See the issue for details: #695.
saig0 added a commit that referenced this issue Sep 7, 2023
The built-in function `is defined()` doesn't work anymore. By returning `null` for a non-existing variable, the function returns always `true`. Previously, the function was used to check if a variable or context entry exists.

See the issue for details: #695.
saig0 added a commit that referenced this issue Sep 7, 2023
The built-in function `is defined()` doesn't work anymore. By returning `null` for a non-existing variable, the function returns always `true`. Previously, the function was used to check if a variable or context entry exists.

See the issue for details: #695.
nicpuppa added a commit that referenced this issue Sep 8, 2023
Ignore test cases due to:
#695
@saig0
Copy link
Member Author

saig0 commented Sep 8, 2023

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.

  • If the variable or the context entry doesn't exist, the function would return false. Like in version 1.17.0. ✔️
  • If the variable or the context entry exists but is null, the function would return false. Different to version 1.17.0. ❌
// === 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: Camunda 8 Required in the context of Camunda 8 target:8.3 Planned for the Camunda 8.3 release type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants