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

fix: Unary-test behavior with special input variable ? #887

Merged
merged 9 commits into from
Aug 23, 2024

Conversation

saig0
Copy link
Member

@saig0 saig0 commented Aug 20, 2024

Description

Fix the behavior of unary-test expressions if the special input variable ? is used to access the input value. From the following definition, the current behavior is incorrect in case d).

image

Key idea:

  • Distinguish the case d) from the other cases
  • Limit the access to the special input variable ? to unary-tests
  • Introduce a new error value type to detect the access of ? and interrupt the evaluation

Reviewer hint:
The first 3 commits are the most relevant. The rest contains a lot of refactoring and may be harder to compare with the original code.

Related issues

closes #864

@saig0 saig0 force-pushed the 864-unary-test-implicit-null branch from 41f403d to 329017d Compare August 20, 2024 13:06
@saig0 saig0 marked this pull request as ready for review August 20, 2024 13:14
Add test cases to explicitly verify that a unary-test expression returns the correct output (true/false/null).

Restructure existing tests cases and group them into the different behaviors.
Introduce a new type for not recoverable errors. Compared to the existing error, this new error should stop the evaluation instead of continue the evaluation with null.
Correct the behavior of unary-test expression to return only true if an expression with the special variable `?` evaluates to true.

Identify the usage by returning a fatal error if `?` is used outside of an unary-test.
Adjust all operations to return a fatal error instead of returning null. This is required to limit the access to the special input symbol `?` outside an unary-tests.

Restructure some helper methods to restrict the handling of fatal errors to a few places. This should help to reduce possible problems with fatal errors.

Move some methods to group different aspects together.
Add a test case to verify that the input variable is not available outside of an unary-test expression.
@saig0 saig0 force-pushed the 864-unary-test-implicit-null branch from 329017d to 63efe64 Compare August 20, 2024 13:15
@saig0 saig0 added the follow-up: camunda docs The PR requires an update of the Camunda documentation. label Aug 21, 2024
Copy link
Contributor

@remcowesterhoud remcowesterhoud left a comment

Choose a reason for hiding this comment

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

Thanks @saig0. I think I understand what's happening but I raised 1 question to be sure. Please have a look 🙂

@saig0 saig0 added this pull request to the merge queue Aug 23, 2024
Merged via the queue into main with commit 0fddf7d Aug 23, 2024
5 checks passed
@saig0 saig0 deleted the 864-unary-test-implicit-null branch August 23, 2024 12:34
@backport-action
Copy link
Collaborator

Backport failed for 1.16, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 1.16
git worktree add -d .worktree/backport-887-to-1.16 origin/1.16
cd .worktree/backport-887-to-1.16
git switch --create backport-887-to-1.16
git cherry-pick -x 866ce0098c40f05ba9da3973f88a008dd9885621 04abac917890a5b0ec27868f710495ade41f0bbc 61cb1696cb241e72717e2eaad211043e855a9bab c9672081221a1562a704915f6631c900e855706c 1ba21cad18170b20cd847e3956e989267e708c2b f171ebf299b3969ccab03236843fa19debbc9934 8354d1b136ea8dc57cc1aa83223ed419ad3f198d 63efe64def6ba866edfb3716141299f82354bb80 ac4737b5db5185146dfbaaee6044b65fc81d9c1f

@backport-action
Copy link
Collaborator

Successfully created backport PR for 1.17:

github-merge-queue bot referenced this pull request in camunda/camunda Sep 3, 2024
#21884)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [org.camunda.feel:feel-engine](http://www.camunda.org)
([source](https://redirect.github.com/camunda/feel-scala)) |
`1.18.0-alpha1` -> `1.18.0` |
[![age](https://developer.mend.io/api/mc/badges/age/maven/org.camunda.feel:feel-engine/1.18.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/maven/org.camunda.feel:feel-engine/1.18.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/maven/org.camunda.feel:feel-engine/1.18.0-alpha1/1.18.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/maven/org.camunda.feel:feel-engine/1.18.0-alpha1/1.18.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>camunda/feel-scala (org.camunda.feel:feel-engine)</summary>

###
[`v1.18.0`](https://redirect.github.com/camunda/feel-scala/releases/tag/1.18.0)

[Compare
Source](https://redirect.github.com/camunda/feel-scala/compare/1.18.0-alpha1...1.18.0)

#### What's Changed

##### Features ✨

- feat: Add built-in function to strip whitespace by
[@&#8203;skayliu](https://redirect.github.com/skayliu) in
[https://github.com/camunda/feel-scala/pull/826](https://redirect.github.com/camunda/feel-scala/pull/826)
- feat: Add built-in function to generate a UUID string by
[@&#8203;skayliu](https://redirect.github.com/skayliu) in
[https://github.com/camunda/feel-scala/pull/834](https://redirect.github.com/camunda/feel-scala/pull/834)
- feat: Add built-in function to encode a string as base64 by
[@&#8203;sbuettner](https://redirect.github.com/sbuettner) in
[https://github.com/camunda/feel-scala/pull/847](https://redirect.github.com/camunda/feel-scala/pull/847)
- feat: Add built-in function to check if a list is empty by
[@&#8203;skayliu](https://redirect.github.com/skayliu) in
[https://github.com/camunda/feel-scala/pull/851](https://redirect.github.com/camunda/feel-scala/pull/851)
- feat: Make the FeelEngineBuilder available from Java/Kotlin by
[@&#8203;vicmosin](https://redirect.github.com/vicmosin) in
[https://github.com/camunda/feel-scala/pull/832](https://redirect.github.com/camunda/feel-scala/pull/832)
- feat: I can interrupt an expression evaluation by
[@&#8203;vicmosin](https://redirect.github.com/vicmosin) in
[https://github.com/camunda/feel-scala/pull/824](https://redirect.github.com/camunda/feel-scala/pull/824)

##### Fixes 🐛

- fix: Regression in string() function for string and null values by
[@&#8203;saig0](https://redirect.github.com/saig0) in
[https://github.com/camunda/feel-scala/pull/749](https://redirect.github.com/camunda/feel-scala/pull/749)
- fix: String escape characters by
[@&#8203;nicpuppa](https://redirect.github.com/nicpuppa) in
[https://github.com/camunda/feel-scala/pull/750](https://redirect.github.com/camunda/feel-scala/pull/750)
- fix: Double quotes in string literal are escaped by
[@&#8203;saig0](https://redirect.github.com/saig0) in
[https://github.com/camunda/feel-scala/pull/780](https://redirect.github.com/camunda/feel-scala/pull/780)
- fix: Variable resolution for custom context by
[@&#8203;saig0](https://redirect.github.com/saig0) in
[https://github.com/camunda/feel-scala/pull/786](https://redirect.github.com/camunda/feel-scala/pull/786)
- fix: `string()` function can handle a context with custom value types
by [@&#8203;mustafadagher](https://redirect.github.com/mustafadagher) in
[https://github.com/camunda/feel-scala/pull/833](https://redirect.github.com/camunda/feel-scala/pull/833)
- fix: Invoke `context()` function with a list containing a custom
context by [@&#8203;saig0](https://redirect.github.com/saig0) in
[https://github.com/camunda/feel-scala/pull/858](https://redirect.github.com/camunda/feel-scala/pull/858)
- fix: `substring()` with exceeding length by
[@&#8203;saig0](https://redirect.github.com/saig0) in
[https://github.com/camunda/feel-scala/pull/893](https://redirect.github.com/camunda/feel-scala/pull/893)
- fix: `number()` to return null if the given string is invalid by
[@&#8203;saig0](https://redirect.github.com/saig0) in
[https://github.com/camunda/feel-scala/pull/892](https://redirect.github.com/camunda/feel-scala/pull/892)
- fix: Return `null` if a date is invalid by
[@&#8203;saig0](https://redirect.github.com/saig0) in
[https://github.com/camunda/feel-scala/pull/889](https://redirect.github.com/camunda/feel-scala/pull/889)
- fix: Function `context put()` works for more than two keys by
[@&#8203;saig0](https://redirect.github.com/saig0) in
[https://github.com/camunda/feel-scala/pull/902](https://redirect.github.com/camunda/feel-scala/pull/902)
- fix: Unary-test behavior with special input variable `?` by
[@&#8203;saig0](https://redirect.github.com/saig0) in
[https://github.com/camunda/feel-scala/pull/887](https://redirect.github.com/camunda/feel-scala/pull/887)
- fix: List operation returns null if the value is not a list by
[@&#8203;saig0](https://redirect.github.com/saig0) in
[https://github.com/camunda/feel-scala/pull/895](https://redirect.github.com/camunda/feel-scala/pull/895)
- fix: Variable name starting with "in" by
[@&#8203;saig0](https://redirect.github.com/saig0) in
[https://github.com/camunda/feel-scala/pull/909](https://redirect.github.com/camunda/feel-scala/pull/909)
- fix: Parse expression with NBSP character by
[@&#8203;saig0](https://redirect.github.com/saig0) in
[https://github.com/camunda/feel-scala/pull/914](https://redirect.github.com/camunda/feel-scala/pull/914)
- fix: Detect duplicated context values in `distinct values()` +
`union()` + `duplicate values()` by
[@&#8203;saig0](https://redirect.github.com/saig0) in
[https://github.com/camunda/feel-scala/pull/908](https://redirect.github.com/camunda/feel-scala/pull/908)

##### Dependencies 🤖

- chore(deps): bump org.apache.maven.plugins:maven-shade-plugin from
3.5.0 to 3.5.1 by
[@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/733](https://redirect.github.com/camunda/feel-scala/pull/733)
- build: Update mvn-scalafmt plugin by
[@&#8203;saig0](https://redirect.github.com/saig0) in
[https://github.com/camunda/feel-scala/pull/721](https://redirect.github.com/camunda/feel-scala/pull/721)
- chore(deps-dev): bump version.log4j from 2.20.0 to 2.21.0 by
[@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/745](https://redirect.github.com/camunda/feel-scala/pull/745)
- chore(deps): bump org.apache.maven.plugins:maven-surefire-plugin from
3.1.2 to 3.2.1 by
[@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/755](https://redirect.github.com/camunda/feel-scala/pull/755)
- chore(deps-dev): bump version.log4j from 2.21.0 to 2.21.1 by
[@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/754](https://redirect.github.com/camunda/feel-scala/pull/754)
- chore(deps): bump org.apache.maven.plugins:maven-surefire-plugin from
3.2.1 to 3.2.2 by
[@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/760](https://redirect.github.com/camunda/feel-scala/pull/760)
- chore(deps-dev): bump version.log4j from 2.21.1 to 2.22.0 by
[@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/764](https://redirect.github.com/camunda/feel-scala/pull/764)
- chore(deps): bump
org.apache.maven.plugins:maven-project-info-reports-plugin from 3.4.5 to
3.5.0 by [@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/766](https://redirect.github.com/camunda/feel-scala/pull/766)
- chore(deps): bump org.apache.maven.plugins:maven-surefire-plugin from
3.2.2 to 3.2.3 by
[@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/775](https://redirect.github.com/camunda/feel-scala/pull/775)
- chore(deps-dev): bump version.log4j from 2.22.0 to 2.22.1 by
[@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/787](https://redirect.github.com/camunda/feel-scala/pull/787)
- chore(deps): bump org.apache.maven.plugins:maven-surefire-plugin from
3.2.3 to 3.2.5 by
[@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/793](https://redirect.github.com/camunda/feel-scala/pull/793)
- chore(deps): bump actions/cache from 3 to 4 by
[@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/795](https://redirect.github.com/camunda/feel-scala/pull/795)
- chore(deps): bump org.apache.maven.plugins:maven-shade-plugin from
3.5.1 to 3.5.2 by
[@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/812](https://redirect.github.com/camunda/feel-scala/pull/812)
- chore(deps-dev): bump version.log4j from 2.22.1 to 2.23.0 by
[@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/811](https://redirect.github.com/camunda/feel-scala/pull/811)
- chore(deps): bump org.scala-lang:scala-library from 2.13.12 to 2.13.13
by [@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/813](https://redirect.github.com/camunda/feel-scala/pull/813)
- chore(deps-dev): bump version.log4j from 2.23.0 to 2.23.1 by
[@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/816](https://redirect.github.com/camunda/feel-scala/pull/816)
- chore(deps): bump org.apache.maven.plugins:maven-assembly-plugin from
3.6.0 to 3.7.0 by
[@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/817](https://redirect.github.com/camunda/feel-scala/pull/817)
- chore(deps): bump org.apache.maven.plugins:maven-assembly-plugin from
3.7.0 to 3.7.1 by
[@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/819](https://redirect.github.com/camunda/feel-scala/pull/819)
- chore(deps): bump com.lihaoyi:fastparse\_2.13 from 3.0.2 to 3.1.0 by
[@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/827](https://redirect.github.com/camunda/feel-scala/pull/827)
- chore(deps): bump org.apache.maven.plugins:maven-jar-plugin from 3.3.0
to 3.4.0 by [@&#8203;dependabot](https://redirect.github.com/dependabot)
in
[https://github.com/camunda/feel-scala/pull/828](https://redirect.github.com/camunda/feel-scala/pull/828)
- chore(deps): bump org.apache.maven.plugins:maven-jar-plugin from 3.4.0
to 3.4.1 by [@&#8203;dependabot](https://redirect.github.com/dependabot)
in
[https://github.com/camunda/feel-scala/pull/837](https://redirect.github.com/camunda/feel-scala/pull/837)
- chore(deps): bump org.apache.maven.plugins:maven-shade-plugin from
3.5.2 to 3.5.3 by
[@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/842](https://redirect.github.com/camunda/feel-scala/pull/842)
- chore(deps): bump net.alchim31.maven:scala-maven-plugin from 4.8.1 to
4.9.0 by [@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/841](https://redirect.github.com/camunda/feel-scala/pull/841)
- chore(deps): bump org.scala-lang:scala-library from 2.13.13 to 2.13.14
by [@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/845](https://redirect.github.com/camunda/feel-scala/pull/845)
- chore(deps): bump net.alchim31.maven:scala-maven-plugin from 4.9.0 to
4.9.1 by [@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/849](https://redirect.github.com/camunda/feel-scala/pull/849)
- chore(deps): bump org.apache.maven.plugins:maven-shade-plugin from
3.5.3 to 3.6.0 by
[@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/862](https://redirect.github.com/camunda/feel-scala/pull/862)
- chore(deps): bump com.fasterxml.uuid:java-uuid-generator from 5.0.0 to
5.1.0 by [@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/863](https://redirect.github.com/camunda/feel-scala/pull/863)
- chore(deps): bump
org.apache.maven.plugins:maven-project-info-reports-plugin from 3.5.0 to
3.6.0 by [@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/868](https://redirect.github.com/camunda/feel-scala/pull/868)
- chore(deps): bump org.apache.maven.plugins:maven-surefire-plugin from
3.2.5 to 3.3.0 by
[@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/866](https://redirect.github.com/camunda/feel-scala/pull/866)
- chore(deps): bump org.apache.maven.plugins:maven-jar-plugin from 3.4.1
to 3.4.2 by [@&#8203;dependabot](https://redirect.github.com/dependabot)
in
[https://github.com/camunda/feel-scala/pull/869](https://redirect.github.com/camunda/feel-scala/pull/869)
- chore(deps): bump
org.apache.maven.plugins:maven-project-info-reports-plugin from 3.6.0 to
3.6.1 by [@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/871](https://redirect.github.com/camunda/feel-scala/pull/871)
- chore(deps): bump net.alchim31.maven:scala-maven-plugin from 4.9.1 to
4.9.2 by [@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/872](https://redirect.github.com/camunda/feel-scala/pull/872)
- chore(deps): bump com.lihaoyi:fastparse\_2.13 from 3.1.0 to 3.1.1 by
[@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/875](https://redirect.github.com/camunda/feel-scala/pull/875)
- chore(deps): bump
org.apache.maven.plugins:maven-project-info-reports-plugin from 3.6.1 to
3.6.2 by [@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/878](https://redirect.github.com/camunda/feel-scala/pull/878)
- chore(deps): bump org.apache.maven.plugins:maven-surefire-plugin from
3.3.0 to 3.3.1 by
[@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/876](https://redirect.github.com/camunda/feel-scala/pull/876)
- chore(deps): bump org.apache.maven.plugins:maven-site-plugin from
3.12.1 to 3.20.0 by
[@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/886](https://redirect.github.com/camunda/feel-scala/pull/886)
- chore(deps): bump org.apache.maven.plugins:maven-surefire-plugin from
3.3.1 to 3.4.0 by
[@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/885](https://redirect.github.com/camunda/feel-scala/pull/885)
- chore(deps): bump
org.apache.maven.plugins:maven-project-info-reports-plugin from 3.6.2 to
3.7.0 by [@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/894](https://redirect.github.com/camunda/feel-scala/pull/894)
- chore(deps): bump org.apache.maven.plugins:maven-surefire-plugin from
3.4.0 to 3.5.0 by
[@&#8203;dependabot](https://redirect.github.com/dependabot) in
[https://github.com/camunda/feel-scala/pull/910](https://redirect.github.com/camunda/feel-scala/pull/910)

#### New Contributors

- [@&#8203;lzgabel](https://redirect.github.com/lzgabel) made their
first contribution in
[https://github.com/camunda/feel-scala/pull/734](https://redirect.github.com/camunda/feel-scala/pull/734)
- [@&#8203;jonathanlukas](https://redirect.github.com/jonathanlukas)
made their first contribution in
[https://github.com/camunda/feel-scala/pull/794](https://redirect.github.com/camunda/feel-scala/pull/794)
- [@&#8203;vicmosin](https://redirect.github.com/vicmosin) made their
first contribution in
[https://github.com/camunda/feel-scala/pull/824](https://redirect.github.com/camunda/feel-scala/pull/824)
- [@&#8203;skayliu](https://redirect.github.com/skayliu) made their
first contribution in
[https://github.com/camunda/feel-scala/pull/826](https://redirect.github.com/camunda/feel-scala/pull/826)
- [@&#8203;mustafadagher](https://redirect.github.com/mustafadagher)
made their first contribution in
[https://github.com/camunda/feel-scala/pull/833](https://redirect.github.com/camunda/feel-scala/pull/833)
- [@&#8203;sbuettner](https://redirect.github.com/sbuettner) made their
first contribution in
[https://github.com/camunda/feel-scala/pull/847](https://redirect.github.com/camunda/feel-scala/pull/847)
- [@&#8203;andromaqui](https://redirect.github.com/andromaqui) made
their first contribution in
[https://github.com/camunda/feel-scala/pull/870](https://redirect.github.com/camunda/feel-scala/pull/870)

**Full Changelog**:
camunda/feel-scala@1.17.0...1.18.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend Renovate](https://mend.io/renovate/).
View the [repository job
log](https://developer.mend.io/github/camunda/camunda).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC41OS4yIiwidXBkYXRlZEluVmVyIjoiMzguNTkuMiIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiYXV0b21lcmdlIl19-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.16 backport 1.17 follow-up: camunda docs The PR requires an update of the Camunda documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Incorrect/inconsistent unary test evaluation when nulls are involved.
3 participants