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

feat: Handle non-existing variables #705

Merged
merged 20 commits into from
Sep 8, 2023
Merged

feat: Handle non-existing variables #705

merged 20 commits into from
Sep 8, 2023

Conversation

saig0
Copy link
Member

@saig0 saig0 commented Sep 7, 2023

Description

Change the behavior of the FEEL engine to replace a non-existing variable and a non-existing input value with null. Previously, the evaluation failed in these cases.

A note to the reviewer, the actual code fix is very small. The major changes are refactorings in the test classes to adopt the new test style.

Related issues

closes #674

saig0 added 19 commits September 5, 2023 13:53
Change the base test class to FeelEngineTest with EvaluationResultMatchers. This aligns with the new test style and improves the readability.
Change the behavior to return null if a referenced variable doesn't exist. Additionally, the evaluation reports a suppressed failure for debugging purpose.

Previously, the evaluation failed if a variable doesn't exist.

This behavioral change is aligned with the DMN specification and improves the handling of null and non-existing variables.
The test case is now working because the missing variable is replaced with null. We can be enabled the test case.
Change the behavior to return null if the input value of a unary-tests expression doesn't exist.

Previously, the evaluation failed if the input value doesn't exist.
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.
This test case doesn't work anymore because the evaluation doesn't fail if the input value is missing. Instead, the input value is replaced with `null`.
These test cases don't work anymore because the evaluation doesn't fail if the input value is missing. Instead, the input value is replaced with `null`.
This test case had a wrong assumption. Previously, the test case failed because of the missing input value but not because the parsing.

Since the evaluation doesn't fail anymore with a missing input value, we can remove the test case.
@saig0 saig0 requested a review from nicpuppa September 7, 2023 10:00
Copy link
Contributor

@nicpuppa nicpuppa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love the refactor of the tests ❤️

LGTM 💪 🚀

I just leave some thoughts, please have a look if you want 👀

@saig0 saig0 merged commit ad09f93 into main Sep 8, 2023
@saig0 saig0 deleted the 674-non-existing-variable branch September 8, 2023 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle non-existing variables and context entries gracefully
2 participants