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

[spearsandshields]

Describe the bug There is some inconsistent handling of null when evaluating unary tests. I will try to demonstrate below using real examples and screenshots.

1. evaluating null > 20
   1. when input value is a random number, this evaluates to false.

2. when input value is null. I would expect the same (since the unary test is not a simple unary test and is not using the input value at all). However, we are getting true. null > 20 => true? This seemed hard to reason about.

2. evaluating ? > 20, and when input value is null. However, when i run in expression mode, or using feelin engine, I would see this evaluate to null. If I run it against dsntk, I would get null as the return value as well.

3. Technically speaking DMN spec did not talk about the handling of null when inequality comparison, however based on the guidance over equality comparison, it seems reasonable to suggest that if the two values are not of the same kind, we should expect null as the return value.

To Reproduce Steps to reproduce the behavior: 1.Type in the values as shown in the screenshot in corresponding playground sites. or go to link to spot the inconsistency https://camunda.github.io/feel-scala/docs/playground/?expression-type=unary-tests&expression=PyA%2BIDIw&context=ewp9&input-value=bnVsbA%3D%3D https://nikku.github.io/feel-playground/?e=%3F+%3E+20&c=%7B%0A++%22%3F%22%3A+null%0A%7D&t=expression&st=true

Expected behavior This is business critical as it directly impact which rule is determined as a match or not a match when nulls are involved in the evaluation context. It seems like the general consensus is to return null, or consider a rule as not a match for unary tests such as ? > 20, when ? is null.

Currently, these null resulting input values are causing unexpected behavior in rule match determination. I think null will be a more reasonable/spec-compliant return value.

Environment

• FEEL engine version: [1.x.y]
• Affects:
  • Camunda Automation Platform 7: [7.x]
  • Zeebe broker: [0.x]

Related SUPPORT-22286

[spearsandshields]

Hi @saig0 @nikku Would you agree that this inconsistency should be addressed in camunda space? Or is there anything I can further explain to clarify the issue?

[saig0]

@spearsandshields thank you for raising the issue and providing details. :+1:

I see your point that the current behavior is not very intuitive. Let's have a look at the DMN specification for the expected behavior:

(DMN 1.5, chapter 7.3.2, page 54)

---

With these rules in mind, the following unary-tests expression should return:

1) Compare with implicit input value

// given
input value = null

// unary-tests expression
> 20

// expected result
false | null

Why?

• case a: the expression > 20 doesn't result in a value
• case b: the expression > 20 doesn't result in a list
• case c: the expression > 20 doesn't result in true but null
• case d: ? is not used
• no case is fulfilled (i.e. the rule doesn't match)

Actual result:

• FEEL-Scala: null :heavy_check_mark:
• Feelin (JS): null :heavy_check_mark:

Note: I don't see a reference if the unary-tests expression should return false or null if none of these cases is fulfilled.

2) Compare with null

// given
input value = null

// unary-tests expression
null > 20

// expected result
true

Why?

• case a: the expression null > 20 results in null and the (implicit) input value is equal to null

Actual result:

• FEEL-Scala: true :heavy_check_mark:
• Feelin (JS): null :heavy_multiplication_x:

3) Compare with ?

// given
input value = null

// unary-tests expression
? > 20

// expected result
false | null

Why?

• by using ?, we can't apply the cases a, b, c
• case a: the expression ? > 20 doesn't result in a value
• case b: the expression ? > 20 doesn't result in a list
• case c: the expression ? > 20 doesn't result in true
• case d: the expression ? > 20 doesn't result in true but null
• no case is fulfilled (i.e. the rule doesn't match)

Actual result:

• FEEL-Scala: true :heavy_multiplication_x:
• Feelin (JS): null :heavy_check_mark:

---

@spearsandshields do you agree with the expectations?

@nikku please have a look at the expectations.

[spearsandshields]

@saig0
I appreciate your thoughtful response.

In short, I tend to agree with all of your understanding here, and your judgments in all these examples are helpful, and meet my updated expectation.

Long story: The only concern I had was Scenario 2. It turns out I got confused with unary test, and forgot that final entry values will be checked against the input value when it does not use ?, and is implicit.

While I agreed with your conclusion, I still wanted to double check this behavior through a different engine.

`<?xml version="1.0" encoding="UTF-8"?> <definitions xmlns="https://www.omg.org/spec/DMN/20191111/MODEL/" xmlns:feel="http://www.omg.org/spec/DMN/20191111/FEEL/" xmlns:dmndi="https://www.omg.org/spec/DMN/20191111/DMNDI/" xmlns:di="https://www.omg.org/spec/DMN/20191111/DI/" xmlns:dc="https://www.omg.org/spec/DMN/20191111/DC/" id="definitions_1" name="UnaryTestsExample" namespace="http://example.com/dmn/unarytestsexample">

InputValue

1 > 2 "Condition null > 2 evaluated" - "Fallback output"

` I made the above simple dmn file, and the results align with your scenario as well.  So hopefully this is additional supporting information for your Scenario 2. A bit more background on why a fix of Scenario 3 will be beneficial. In real world, the context object can be very large, and not all properties guarantees a value. So if the expression involves `?` it would be ideal to not return false positive matches in a comparison such as `? > 20` (the use of ? is necessary for my use case, since this would allow users to put in expressions such as `? > 20 or ? = null` in their DMN rules, and it can make the rules more practical without creating a ton of rows just for null handling, however, with the current bug, the final evaluation returns unexpected results)

[saig0]

@spearsandshields thank you for confirming and providing more details. :+1:

---

the use of ? is necessary for my use case, since this would allow users to put in expressions such as ? > 20 or ? = null in their DMN rules

No, you don't need to use ?. Instead of ? > 20 or ? = null, you could write > 20, null. It is shorter and has the same semantics.

Try in FEEL-Scala Playground.

? is a special symbol for unary-tests expressions that are written as a boolean expression. A common case is the usage of built-in functions, for example, ends with(?, "@camunda.com").

[nikku]

Related issue in feelin (FEEL JS) - https://github.com/nikku/feelin/issues/71

@saig0 https://github.com/camunda/feel-scala/issues/864#issuecomment-2188837091

I subscribe to your analysis.

What would probably help us (and this is on my todo list) is to assert such cases clearly via the DMN TCK. To my knowledge both tools use the TCK for testing and behave differently; this indicates a lack of test coverage :whale:.

[spearsandshields]

@saig0 , thanks for sharing a workaround.

Yes, the comma delimiter is the first route I went with, since it is simpler. Unfortunately, that does not work if there is negation in the unary test. not(X) seemed necessary, since != does not work when it lacks of a left operand?

I could try to diverge and provide different solution when negation if involved, and when it is not. However, that still does not cover the negation case, and it will introduce more complexity. In short, I chose to introduce ? as it provides consistency and simplicity and it covers the negation case. Unsure if I misunderstood anything, but hopefully this argument make sense.

Below is a screenshot. My interpretation is that comma is valid token for positive unary tests only. (which is a bit sad)

All three engines demonstrate the same behavior

PS: there are many examples for the need or negation. For example range checking, not(? in [1,2]) , not equal to, negating a function, such as not(contains(?, 'substring')), not(matches(?, regex)) (when regex is complicated to come up with the negated counterparts) ...

[spearsandshields]

@saig0 I just created a PR to address this issue, would appreciate if you can take a look! https://github.com/camunda/feel-scala/pull/882