search

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

fix: `is defined()` function returns false if the value is null #712

Closed saig0 closed 10 months ago

saig0 commented 10 months ago

Description

Change the behavior of the function is defined() to return true if the value is not null. Previously, the function returned true if the value was null. It returned false if the value was a non-existing variable or context entry (i.e. a ValError).

The changes in the null-handling of the FEEL engine to replace a non-existing variable or context entry with null made this function useless. It returned always true.

A complete fix of the function is not possible because of the new null-handling. However, this change in the semantics of the function reduces the impact on the users (i.e. the regression).

The function still works as before for non-existing variables and context entries. But it may break for variables and context entries with a null value. See here for details.

Related issues

closes #695

saig0 commented 10 months ago

@korthout please review this small PR in the next few days. :cake:

I would like to hear your opinion on the fix and if it fixes the regression sufficiently. Happy to chat about it.

abbasadel commented 10 months ago

Hi @nicpuppa , maybe you can review this PR since Nico is a bit busy right now.

nicpuppa commented 10 months ago

💭 As the new null-handling made this function useless, and every fix introduce a breaking change, is not possible to deprecate it ? 💭 The new behaviour of this function is closer to a null check, so the name is defined() is no more semantically correct, is null() would be more correct imho.

saig0 commented 10 months ago

💭 As the new null-handling made this function useless, and every fix introduce a breaking change, is not possible to deprecate it ?

Yes, but it would have no effect. The function could be used in existing BPMNs or DMN. To avoid breaking these models with the new version, we can't remove the function.

💭 The new behaviour of this function is closer to a null check, so the name is defined() is no more semantically correct, is null() would be more correct imho.

I agree. :sweat_smile: However, we need to keep the existing name to avoid breaking user models.

For the future, I would recommend using a regular null-check instead of the function.

is defined(x)
// vs.
x != null