Merge pull request #908 from camunda/771-distinct-values

fix: Detect duplicated context values in `distinct values()` + `union()` + `duplicate values()`

Author: saig0

src/main/scala/org/camunda/feel/FeelEngine.scala

@@ -119,7 +119,7 @@ class FeelEngine(
     val clock: FeelEngineClock = FeelEngine.defaultClock
 ) {
 
-  private val interpreter = new FeelInterpreter()
+  private val interpreter = new FeelInterpreter(valueMapper)
 
   private val validator = new ExpressionValidator(
     externalFunctionsEnabled = configuration.externalFunctionsEnabled

src/main/scala/org/camunda/feel/impl/builtin/ListBuiltinFunctions.scala

@@ -18,6 +18,7 @@ package org.camunda.feel.impl.builtin
 
 import org.camunda.feel.impl.builtin.BuiltinFunction.builtinFunction
 import org.camunda.feel.Number
+import org.camunda.feel.impl.interpreter.ValComparator
 import org.camunda.feel.syntaxtree.{
   Val,
   ValBoolean,
@@ -28,10 +29,13 @@ import org.camunda.feel.syntaxtree.{
   ValNumber,
   ValString
 }
+import org.camunda.feel.valuemapper.ValueMapper
 
 import scala.annotation.tailrec
 
-object ListBuiltinFunctions {
+class ListBuiltinFunctions(private val valueMapper: ValueMapper) {
+
+  private val valueComparator = new ValComparator(valueMapper)
 
   def functions = Map(
     "list contains"    -> List(listContainsFunction),
@@ -375,32 +379,40 @@ object ListBuiltinFunctions {
   private def unionFunction = builtinFunction(
     params = List("lists"),
     invoke = { case List(ValList(lists)) =>
-      ValList(
-        lists
-          .flatMap(_ match {
-            case ValList(list) => list
-            case v             => List(v)
-          })
-          .toList
-          .distinct
-      )
+      val listOfLists = lists.flatMap {
+        case ValList(list) => list
+        case v             => List(v)
+      }
+      ValList(distinct(listOfLists))
     },
     hasVarArgs = true
   )
 
   private def distinctValuesFunction =
     builtinFunction(
       params = List("list"),
-      invoke = { case List(ValList(list)) =>
-        ValList(list.distinct)
+      invoke = { case List(ValList(list)) => ValList(distinct(list)) }
+    )
+
+  private def distinct(list: List[Val]): List[Val] = {
+    list.foldLeft(List[Val]())((result, item) =>
+      if (result.exists(y => valueComparator.equals(item, y))) {
+        // duplicate value
+        result
+      } else {
+        result :+ item
       }
     )
+  }
 
   private def duplicateValuesFunction =
     builtinFunction(
       params = List("list"),
       invoke = { case List(ValList(list)) =>
-        ValList(list.distinct.filter(x => list.count(_ == x) > 1))
+        val duplicatedValues =
+          distinct(list).filter(x => list.count(valueComparator.equals(_, x)) > 1)
+
+        ValList(duplicatedValues)
       }
     )
 

src/main/scala/org/camunda/feel/impl/interpreter/BuiltinFunctions.scala

@@ -42,7 +42,7 @@ class BuiltinFunctions(clock: FeelEngineClock, valueMapper: ValueMapper) extends
     new ConversionBuiltinFunctions(valueMapper).functions ++
       BooleanBuiltinFunctions.functions ++
       StringBuiltinFunctions.functions ++
-      ListBuiltinFunctions.functions ++
+      new ListBuiltinFunctions(valueMapper).functions ++
       NumericBuiltinFunctions.functions ++
       new ContextBuiltinFunctions(valueMapper).functions ++
       RangeBuiltinFunction.functions ++

src/main/scala/org/camunda/feel/impl/interpreter/FeelInterpreter.scala

@@ -39,7 +39,9 @@ import scala.reflect.ClassTag
 /** @author
   *   Philipp Ossler
   */
-class FeelInterpreter {
+class FeelInterpreter(private val valueMapper: ValueMapper) {
+
+  private val valueComparator = new ValComparator(valueMapper)
 
   def eval(expression: Exp)(implicit context: EvalContext): Val = {
     // Check if the current thread was interrupted, otherwise long-running evaluations can not be interrupted and fully block the thread
@@ -80,7 +82,7 @@ class FeelInterpreter {
 
       // simple unary tests
       case InputEqualTo(x)                              =>
-        withVal(getImplicitInputValue, i => checkEquality(i, eval(x), _ == _, ValBoolean))
+        withVal(getImplicitInputValue, i => checkEquality(i, eval(x)))
       case InputLessThan(x)                             =>
         withVal(getImplicitInputValue, i => dualOp(i, eval(x), _ < _, ValBoolean))
       case InputLessOrEqual(x)                          =>
@@ -118,7 +120,7 @@ class FeelInterpreter {
         withValOrNull(withNumber(eval(x), x => ValNumber(-x)))
 
       // dual comparators
-      case Equal(x, y)          => checkEquality(eval(x), eval(y), _ == _, ValBoolean)
+      case Equal(x, y)          => checkEquality(eval(x), eval(y))
       case LessThan(x, y)       => dualOp(eval(x), eval(y), _ < _, ValBoolean)
       case LessOrEqual(x, y)    => dualOp(eval(x), eval(y), _ <= _, ValBoolean)
       case GreaterThan(x, y)    => dualOp(eval(x), eval(y), _ > _, ValBoolean)
@@ -513,65 +515,15 @@ class FeelInterpreter {
     }
   }
 
-  private def checkEquality(x: Val, y: Val, c: (Any, Any) => Boolean, f: Boolean => Val)(implicit
-      context: EvalContext
-  ): Val =
+  private def checkEquality(x: Val, y: Val)(implicit context: EvalContext): Val =
     withValues(
       x,
       y,
-      {
-        case (ValNull, _)                                       => f(c(ValNull, y.toOption.getOrElse(ValNull)))
-        case (_, ValNull)                                       => f(c(x.toOption.getOrElse(ValNull), ValNull))
-        case (ValNumber(x), ValNumber(y))                       => f(c(x, y))
-        case (ValBoolean(x), ValBoolean(y))                     => f(c(x, y))
-        case (ValString(x), ValString(y))                       => f(c(x, y))
-        case (ValDate(x), ValDate(y))                           => f(c(x, y))
-        case (ValLocalTime(x), ValLocalTime(y))                 => f(c(x, y))
-        case (ValTime(x), ValTime(y))                           => f(c(x, y))
-        case (ValLocalDateTime(x), ValLocalDateTime(y))         => f(c(x, y))
-        case (ValDateTime(x), ValDateTime(y))                   => f(c(x, y))
-        case (ValYearMonthDuration(x), ValYearMonthDuration(y)) => f(c(x, y))
-        case (ValDayTimeDuration(x), ValDayTimeDuration(y))     => f(c(x, y))
-        case (ValList(x), ValList(y))                           =>
-          if (x.size != y.size) {
-            f(false)
-
-          } else {
-            val isEqual = x.zip(y).foldRight(true) { case ((x, y), listIsEqual) =>
-              listIsEqual && {
-                checkEquality(x, y, c, f) match {
-                  case ValBoolean(itemIsEqual) => itemIsEqual
-                  case _                       => false
-                }
-              }
-            }
-            f(isEqual)
-          }
-        case (ValContext(x), ValContext(y))                     =>
-          val xVars = x.variableProvider.getVariables
-          val yVars = y.variableProvider.getVariables
-
-          if (xVars.keys != yVars.keys) {
-            f(false)
-
-          } else {
-            val isEqual = xVars.keys.foldRight(true) { case (key, contextIsEqual) =>
-              contextIsEqual && {
-                val xVal = context.valueMapper.toVal(xVars(key))
-                val yVal = context.valueMapper.toVal(yVars(key))
-
-                checkEquality(xVal, yVal, c, f) match {
-                  case ValBoolean(entryIsEqual) => entryIsEqual
-                  case _                        => false
-                }
-              }
-            }
-            f(isEqual)
-          }
-        case _                                                  =>
+      (x, y) =>
+        valueComparator.compare(x, y).toOption.getOrElse {
           error(EvaluationFailureType.NOT_COMPARABLE, s"Can't compare '$x' with '$y'")
           ValNull
-      }
+        }
     )
 
   private def dualOp(x: Val, y: Val, c: (Val, Val) => Boolean, f: Boolean => Val)(implicit
@@ -715,7 +667,7 @@ class FeelInterpreter {
             // the expression contains the input value
             ValBoolean(true)
           case x                                      =>
-            checkEquality(inputValue, x, _ == _, ValBoolean) match {
+            checkEquality(inputValue, x) match {
               case ValBoolean(true)             =>
                 // the expression is the input value
                 ValBoolean(true)

src/main/scala/org/camunda/feel/impl/interpreter/ValComparator.scala

@@ -0,0 +1,69 @@
+/*
+ * Copyright Camunda Services GmbH and/or licensed to Camunda Services GmbH
+ * under one or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information regarding copyright
+ * ownership. Camunda licenses this file to you under the Apache License,
+ * Version 2.0; you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.camunda.feel.impl.interpreter
+
+import org.camunda.feel.context.Context
+import org.camunda.feel.syntaxtree._
+import org.camunda.feel.valuemapper.ValueMapper
+
+class ValComparator(private val valueMapper: ValueMapper) {
+
+  def equals(x: Val, y: Val): Boolean = compare(x, y) match {
+    case ValBoolean(isEqual) => isEqual
+    case _                   => false
+  }
+
+  def compare(x: Val, y: Val): Val = (x, y) match {
+    // both values are null
+    case (ValNull, _)                                       => ValBoolean(ValNull == y.toOption.getOrElse(ValNull))
+    case (_, ValNull)                                       => ValBoolean(x.toOption.getOrElse(ValNull) == ValNull)
+    // compare values of the same type
+    case (ValNumber(x), ValNumber(y))                       => ValBoolean(x == y)
+    case (ValBoolean(x), ValBoolean(y))                     => ValBoolean(x == y)
+    case (ValString(x), ValString(y))                       => ValBoolean(x == y)
+    case (ValDate(x), ValDate(y))                           => ValBoolean(x == y)
+    case (ValLocalTime(x), ValLocalTime(y))                 => ValBoolean(x == y)
+    case (ValTime(x), ValTime(y))                           => ValBoolean(x == y)
+    case (ValLocalDateTime(x), ValLocalDateTime(y))         => ValBoolean(x == y)
+    case (ValDateTime(x), ValDateTime(y))                   => ValBoolean(x == y)
+    case (ValYearMonthDuration(x), ValYearMonthDuration(y)) => ValBoolean(x == y)
+    case (ValDayTimeDuration(x), ValDayTimeDuration(y))     => ValBoolean(x == y)
+    case (ValList(x), ValList(y))                           => compare(x, y)
+    case (ValContext(x), ValContext(y))                     => compare(x, y)
+    // values have a different type
+    case _                                                  => ValError(s"Can't compare '$x' with '$y'")
+  }
+
+  private def compare(x: List[Val], y: List[Val]): ValBoolean = {
+    ValBoolean(
+      x.size == y.size && x.zip(y).forall { case (itemX, itemY) => equals(itemX, itemY) }
+    )
+  }
+
+  private def compare(x: Context, y: Context): ValBoolean = {
+    val xVars = x.variableProvider.getVariables
+    val yVars = y.variableProvider.getVariables
+
+    ValBoolean(xVars.keys == yVars.keys && xVars.keys.forall { key =>
+      val xVal = valueMapper.toVal(xVars(key))
+      val yVal = valueMapper.toVal(yVars(key))
+
+      equals(xVal, yVal)
+    })
+  }
+
+}

src/test/scala/org/camunda/feel/impl/FeelIntegrationTest.scala

@@ -37,7 +37,7 @@ import org.camunda.feel.{
 trait FeelIntegrationTest {
 
   val interpreter: FeelInterpreter =
-    new FeelInterpreter
+    new FeelInterpreter(ValueMapper.defaultValueMapper)
 
   private val clock: TimeTravelClock = new TimeTravelClock