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

A missing variable/nested property can not be handled #4212

Closed
saig0 opened this issue Apr 2, 2020 · 9 comments · Fixed by #5392
Closed

A missing variable/nested property can not be handled #4212

saig0 opened this issue Apr 2, 2020 · 9 comments · Fixed by #5392
Assignees
Labels
kind/bug Categorizes an issue or PR as a bug scope/broker Marks an issue or PR to appear in the broker section of the changelog severity/mid Marks a bug as having a noticeable impact but with a known workaround

Comments

@saig0
Copy link
Member

saig0 commented Apr 2, 2020

Describe the bug
In an expression, a missing variable or a non-existing nested property can not be handled. It is handled as a failure (i.e. ValError) in the FEEL engine. Some boolean expressions, like an equal comparison (e.g. = x = "here"), can deal with it and returns false.

But it is not possible to check if a value/property is missing using the null value. For example,

not_existing = null
// false

not_existing != null
// true

This behavior is not intuitive.

To Reproduce

  • workflow with an exclusive gateway with a condition x != null
  • create a workflow instance without variables
  • verify that the sequence flow with the condition is taken

Expected behavior
A missing variable/nested property can be compared to null and returns true.

Alternatively, provide a method to check if a variable/property is not present (e.g. isPresent(x)).

Environment:

  • OS: [e.g. Linux]
  • Zeebe Version: 0.23.0-SNAPSHOT
  • Configuration: [e.g. exporters etc.]
@saig0 saig0 added Status: Planned kind/bug Categorizes an issue or PR as a bug scope/broker Marks an issue or PR to appear in the broker section of the changelog labels Apr 2, 2020
@saig0 saig0 added this to the Advanced expression supported milestone Apr 2, 2020
@saig0 saig0 removed this from the Advanced expression supported milestone Apr 20, 2020
@npepinpe npepinpe added severity/mid Marks a bug as having a noticeable impact but with a known workaround Priority: Mid and removed Status: Planned labels Apr 28, 2020
@saig0
Copy link
Member Author

saig0 commented Jun 9, 2020

Current workaround:

  • check if a variable has a specific type
if(x instance of string) then x else "default"

@korthout
Copy link
Member

korthout commented Jul 7, 2020

I'd like to re-evaluate the priority of this issue.

@asaf
Copy link

asaf commented Jul 11, 2020

@korthout this is a big issue, we avoided upgrading because of this.

some examples we ended up with but felt like it's way to compex to be used:

=(get value (products, "lists") instance of list and count(get value (product, "lists")) > 0
=not(get value(person.org, "manager") instance of context

btw, in general, it's very annoying that Zeebe fails when accessing a nonexisting variable, this makes it very hard to ask if "foo.bar.baz" exists in gateways and such.

Thanks.

@saig0
Copy link
Member Author

saig0 commented Jul 13, 2020

@asaf, thank you for your input. We'll work on this soon.

My idea is to use the null check for non-existing variables and variables with null values. This should work also for nested properties if the property or the variables doesn't exist.

x = null 
// true if x is null or doesn't exist

x.y = null  
// true if x is null or doesn't exist
// true if y is null or doesn't exist

If the value or nested property is accessed without null check then an incident can be raised.

Do you have any proposals for your other examples?

@asaf
Copy link

asaf commented Jul 13, 2020

@saig0 I think that fixing null equality is top priority
and safe get (e.g., "x.y.z") even if x or y does not exist will solve most of the issues

If you can't do it, then at least get value (obj, "x.y.z") should do the trick

@asaf
Copy link

asaf commented Aug 14, 2020

@saig0 per your last comment, please pay attention that in case y does not exist on x an expr such:

x.y != null

will cause an error that x context does not contain y.

this behavior blocks checking x.y == null

@saig0
Copy link
Member Author

saig0 commented Aug 25, 2020

Open PR: camunda/feel-scala#176

@npepinpe
Copy link
Member

Will this be fixed by #5355?

@saig0
Copy link
Member Author

saig0 commented Sep 22, 2020

I'll open a new PR. I need to add some integration code :)

@zeebe-bors zeebe-bors bot closed this as completed in cd1ebb2 Sep 24, 2020
zeebe-bors bot added a commit that referenced this issue Sep 28, 2020
5432: [Backport 0.24] Fix flaky test: MultiInstanceActivityTest.shouldTriggerInterruptingBoundaryEvent r=saig0 a=korthout

##  Description

Backports: #5427

Fix flaky test: MultiInstanceActivityTest.shouldTriggerInterruptingBoundaryEvent

## Related issues

closes #5097

## Definition of Done

_Not all items need to be done depending on the issue and the pull request._

Code changes:
* [ ] The changes are backwards compatibility with previous versions
* [x] If it fixes a bug then PRs are created to [backport](https://github.com/zeebe-io/zeebe/compare/stable/0.24...develop?expand=1&template=backport_template.md&title=[Backport%200.24]) the fix to the last two minor versions

Testing:
* [ ] There are unit/integration tests that verify all acceptance criterias of the issue
* [ ] New tests are written to ensure backwards compatibility with further versions
* [ ] The behavior is tested manually
* [ ] The impact of the changes is verified by a benchmark 

Documentation: 
* [ ] The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
* [ ] New content is added to the [release announcement](https://drive.google.com/drive/u/0/folders/1DTIeswnEEq-NggJ25rm2BsDjcCQpDape)


Co-authored-by: Nico korthout <nico.korthout@camunda.com>

## Description

<!-- Please explain the changes you made here. -->

## Related issues

<!-- Which issues are closed by this PR or are related -->

closes #

## Definition of Done

_Not all items need to be done depending on the issue and the pull request._

Code changes:
* [ ] The changes are backwards compatibility with previous versions
* [ ] If it fixes a bug then PRs are created to [backport](https://github.com/zeebe-io/zeebe/compare/stable/0.24...develop?expand=1&template=backport_template.md&title=[Backport%200.24]) the fix to the last two minor versions

Testing:
* [ ] There are unit/integration tests that verify all acceptance criterias of the issue
* [ ] New tests are written to ensure backwards compatibility with further versions
* [ ] The behavior is tested manually
* [ ] The impact of the changes is verified by a benchmark 

Documentation: 
* [ ] The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
* [ ] New content is added to the [release announcement](https://drive.google.com/drive/u/0/folders/1DTIeswnEEq-NggJ25rm2BsDjcCQpDape)


5440: [Backport 0.24] Dump feel-scala from 1.11.2 to 1.12.2 r=saig0 a=saig0

## Description

Backport of #5392

## Related issues

closes #5140
closes #4212


Co-authored-by: Nico korthout <nico.korthout@camunda.com>
Co-authored-by: Philipp Ossler <philipp.ossler@gmail.com>
zeebe-bors bot added a commit that referenced this issue Sep 28, 2020
5441: [Backport 0.23] Dump feel-scala from 1.11.0 to 1.12.2 r=saig0 a=saig0

## Description

Backport of #5392

## Related issues

closes #5140
closes #4212


Co-authored-by: Philipp Ossler <philipp.ossler@gmail.com>
zeebe-bors bot added a commit that referenced this issue Sep 29, 2020
5441: [Backport 0.23] Dump feel-scala from 1.11.0 to 1.12.2 r=saig0 a=saig0

## Description

Backport of #5392

## Related issues

closes #5140
closes #4212


Co-authored-by: Philipp Ossler <philipp.ossler@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes an issue or PR as a bug scope/broker Marks an issue or PR to appear in the broker section of the changelog severity/mid Marks a bug as having a noticeable impact but with a known workaround
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants