Merge remote-tracking branch 'origin/staging' into change/NU-1777

components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/AssignabilityDeterminer.scala

@@ -32,6 +32,9 @@ object AssignabilityDeterminer {
   def isAssignableStrict(from: TypingResult, to: TypingResult): ValidatedNel[String, Unit] =
     isAssignable(from, to, StrictConversionChecker)
 
+  def isAssignableWithoutConversion(from: TypingResult, to: TypingResult): ValidatedNel[String, Unit] =
+    isAssignable(from, to, WithoutConversionChecker)
+
   private def isAssignable(from: TypingResult, to: TypingResult, conversionChecker: ConversionChecker) = {
     (from, to) match {
       case (_, Unknown)       => ().validNel
@@ -223,6 +226,19 @@ object AssignabilityDeterminer {
 
   }
 
+  private object WithoutConversionChecker extends ConversionChecker {
+
+    override def isConvertable(
+        from: SingleTypingResult,
+        to: TypedClass
+    ): ValidatedNel[String, Unit] = {
+      val errMsgPrefix =
+        s"${from.runtimeObjType.display} is not the same as ${to.display}"
+      condNel(from.withoutValue == to.withoutValue, (), errMsgPrefix)
+    }
+
+  }
+
   private object StrictConversionChecker extends ConversionChecker {
 
     override def isConvertable(

components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/typing.scala

@@ -40,6 +40,12 @@ object typing {
     final def canBeStrictlyConvertedTo(typingResult: TypingResult): Boolean =
       AssignabilityDeterminer.isAssignableStrict(this, typingResult).isValid
 
+    /**
+     * Checks if the conversion to a given typingResult can be made without any conversion.
+     */
+    final def canBeConvertedWithoutConversionTo(typingResult: TypingResult): Boolean =
+      AssignabilityDeterminer.isAssignableWithoutConversion(this, typingResult).isValid
+
     def valueOpt: Option[Any]
 
     def withoutValue: TypingResult

docs/Changelog.md

@@ -58,6 +58,7 @@
 * [#7364](https://github.com/TouK/nussknacker/pull/7364) PeriodicDeploymentManger is no longer a separate DM, but instead is an optional functionality and decorator for all DMs
   * in order to use it, DM must implement interface `schedulingSupported`, that handles deployments on a specific engine
   * implementation provided for Flink DM
+* [#7443](https://github.com/TouK/nussknacker/pull/7443) Indexing on record is more similar to indexing on map. The change lets us access record values dynamically. For example now spel expression "{a: 5, b: 10}[#input.field]" compiles and has type "Integer" inferred from types of values of the record. This lets us access record value based on user input, for instance if user passes "{"field": "b"}" to scenario we will get value "10", whereas input {"field": "c"} would result in "null". Expression "{a: 5}["b"]" still does not compile because it is known at compile time that record does not have property "b".
 * [#7335](https://github.com/TouK/nussknacker/pull/7335) introduced `managersDirs` config to configure deployment managers directory paths (you can use `MANAGERS_DIR` env in case of docker-based deployments). The default is `./managers`.
 
 ## 1.18

scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpressionParseError.scala

@@ -106,6 +106,10 @@ object SpelExpressionParseError {
       override def message: String = s"There is no property '$property' in type: ${typ.display}"
     }
 
+    case class NoPropertyTypeError(typ: TypingResult, propertyType: TypingResult) extends MissingObjectError {
+      override def message: String = s"There is no property of type '${propertyType.display}' in type: ${typ.display}"
+    }
+
     case class UnknownMethodError(methodName: String, displayableType: String) extends MissingObjectError {
       override def message: String = s"Unknown method '$methodName' in $displayableType"
     }

scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/Typer.scala

@@ -26,6 +26,7 @@ import pl.touk.nussknacker.engine.spel.SpelExpressionParseError.IllegalOperation
 import pl.touk.nussknacker.engine.spel.SpelExpressionParseError.MissingObjectError.{
   ConstructionOfUnknown,
   NoPropertyError,
+  NoPropertyTypeError,
   NonReferenceError,
   UnresolvedReferenceError
 }
@@ -199,11 +200,16 @@ private[spel] class Typer(
             case _                                      => typeFieldNameReferenceOnRecord(indexString, record)
           }
         case indexKey :: Nil if indexKey.canBeConvertedTo(Typed[String]) =>
-          if (dynamicPropertyAccessAllowed) valid(Unknown) else invalid(DynamicPropertyAccessError)
-        case _ :: Nil =>
+          if (dynamicPropertyAccessAllowed) valid(Unknown)
+          else
+            record.runtimeObjType.params match {
+              case _ :: value :: Nil if record.runtimeObjType.klass == classOf[java.util.Map[_, _]] => valid(value)
+              case _                                                                                => valid(Unknown)
+            }
+        case e :: Nil =>
           indexer.children match {
             case (ref: PropertyOrFieldReference) :: Nil => typeFieldNameReferenceOnRecord(ref.getName, record)
-            case _ => if (dynamicPropertyAccessAllowed) valid(Unknown) else invalid(DynamicPropertyAccessError)
+            case _ => if (dynamicPropertyAccessAllowed) valid(Unknown) else invalid(NoPropertyTypeError(record, e))
           }
         case _ =>
           invalid(IllegalIndexingOperation)
@@ -218,7 +224,17 @@ private[spel] class Typer(
           // TODO: validate indexer key - the only valid key is an integer - but its more complicated with references
           withTypedChildren(_ => valid(param))
         case TypedClass(clazz, keyParam :: valueParam :: Nil) if clazz.isAssignableFrom(classOf[java.util.Map[_, _]]) =>
-          withTypedChildren(_ => valid(valueParam))
+          withTypedChildren {
+            // Spel implementation of map indexer (in class org.springframework.expression.spel.ast.Indexer, line 154) tries to convert
+            // indexer to key type of map, but this conversion can be accomplished only if key type of map is known to spel.
+            // Currently .asMap extension is implemented in such a way, that spel does not know key type of the resulting map
+            // (that is when spel evaluates this expression it only knows that it got map, but does not know its type parameters).
+            // It would be hard to change implementation of .asMap extension so we partially turn off this feature of indexer conversion
+            // by allowing in typing only situations when map key type and indexer type are the same (though we have to allow
+            // indexing with unknown type)
+            case indexKey :: Nil if indexKey.canBeConvertedWithoutConversionTo(keyParam) => valid(valueParam)
+            case _ => invalid(IllegalIndexingOperation)
+          }
         case d: TypedDict                    => dictTyper.typeDictValue(d, e).map(toNodeResult)
         case union: TypedUnion               => typeUnion(e, union)
         case TypedTaggedValue(underlying, _) => typeIndexer(e, underlying)

scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSpec.scala

@@ -272,6 +272,18 @@ class SpelExpressionSpec extends AnyFunSuite with Matchers with ValidatedValuesD
   private def evaluate[T: TypeTag](expr: String, context: Context = ctx): T =
     parse[T](expr = expr, context = context).validExpression.evaluateSync[T](context)
 
+  test("should be able to dynamically index record") {
+    evaluate[Int]("{a: 5, b: 10}[#input.toString()]", Context("abc").withVariable("input", "a")) shouldBe 5
+    evaluate[Integer]("{a: 5, b: 10}[#input.toString()]", Context("abc").withVariable("input", "asdf")) shouldBe null
+  }
+
+  test("should figure out result type when dynamically indexing record") {
+    evaluate[Int](
+      "{a: {g: 5, h: 10}, b: {g: 50, h: 100}}[#input.toString()].h",
+      Context("abc").withVariable("input", "b")
+    ) shouldBe 100
+  }
+
   test("parsing first selection on array") {
     parse[Any]("{1,2,3,4,5,6,7,8,9,10}.^[(#this%2==0)]").validExpression
       .evaluateSync[java.util.ArrayList[Int]](ctx) should equal(2)
@@ -897,6 +909,28 @@ class SpelExpressionSpec extends AnyFunSuite with Matchers with ValidatedValuesD
     parse[java.math.BigDecimal]("-1.1", ctx) shouldBe Symbol("valid")
   }
 
+  test("should not validate map indexing if index type and map key type are different") {
+    parse[Any]("""{{key: "a", value: 5}}.toMap[0]""") shouldBe Symbol("invalid")
+    parse[Any]("""{{key: 1, value: 5}}.toMap["0"]""") shouldBe Symbol("invalid")
+    parse[Any]("""{{key: 1.toLong, value: 5}}.toMap[0]""") shouldBe Symbol("invalid")
+    parse[Any]("""{{key: 1, value: 5}}.toMap[0.toLong]""") shouldBe Symbol("invalid")
+  }
+
+  test("should validate map indexing if index type and map key type are the same") {
+    parse[Any]("""{{key: 1, value: 5}}.toMap[0]""") shouldBe Symbol("valid")
+  }
+
+  test("should handle map indexing with unknown key type") {
+    val context = Context("sth").withVariables(
+      Map(
+        "unknownString" -> ContainerOfUnknown("a"),
+      )
+    )
+
+    evaluate[Int]("""{{key: "a", value: 5}}.toMap[#unknownString.value]""", context) shouldBe 5
+    evaluate[Integer]("""{{key: "b", value: 5}}.toMap[#unknownString.value]""", context) shouldBe null
+  }
+
   test("validate ternary operator") {
     parse[Long]("'d'? 3 : 4", ctx) should not be Symbol("valid")
     parse[String]("1 > 2 ? 12 : 23", ctx) should not be Symbol("valid")

scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/TyperSpec.scala

@@ -13,8 +13,10 @@ import pl.touk.nussknacker.engine.api.typed.typing._
 import pl.touk.nussknacker.engine.definition.clazz.ClassDefinitionTestUtils
 import pl.touk.nussknacker.engine.dict.{KeysDictTyper, SimpleDictRegistry}
 import pl.touk.nussknacker.engine.expression.PositionRange
-import pl.touk.nussknacker.engine.spel.SpelExpressionParseError.IllegalOperationError.DynamicPropertyAccessError
-import pl.touk.nussknacker.engine.spel.SpelExpressionParseError.MissingObjectError.NoPropertyError
+import pl.touk.nussknacker.engine.spel.SpelExpressionParseError.MissingObjectError.{
+  NoPropertyError,
+  NoPropertyTypeError
+}
 import pl.touk.nussknacker.engine.spel.SpelExpressionParseError.UnsupportedOperationError.MapWithExpressionKeysError
 import pl.touk.nussknacker.engine.spel.Typer.TypingResultWithContext
 import pl.touk.nussknacker.engine.spel.TyperSpecTestData.TestRecord._
@@ -162,10 +164,8 @@ class TyperSpec extends AnyFunSuite with Matchers with ValidatedValuesDetailedMe
     typeExpression(s"$testRecordExpr[#var]", "var" -> s"$nonPresentKey").invalidValue.toList should matchPattern {
       case NoPropertyError(typingResult, key) :: Nil if typingResult == testRecordTyped && key == nonPresentKey =>
     }
-    // TODO: this behavior is to be fixed - ideally this should behave the same as above
     typeExpression(s"$testRecordExpr[$nonPresentKey]").invalidValue.toList should matchPattern {
-      case NoPropertyError(typingResult, key) :: DynamicPropertyAccessError :: Nil
-          if typingResult == testRecordTyped && key == nonPresentKey =>
+      case NoPropertyError(typingResult, key) :: Nil if typingResult == testRecordTyped && key == nonPresentKey =>
     }
   }
 
@@ -183,6 +183,17 @@ class TyperSpec extends AnyFunSuite with Matchers with ValidatedValuesDetailedMe
     }
   }
 
+  test("indexing on records with key which is not known at compile time treats record as map") {
+    typeExpression("{a: 5, b: 10}[#var.toString()]", "var" -> "a").validValue.finalResult.typingResult shouldBe Typed
+      .typedClass[Int]
+  }
+
+  test("indexing on records with non string key produces error") {
+    typeExpression("{a: 5, b: 10}[4]").invalidValue.toList should matchPattern {
+      case NoPropertyTypeError(_, _) :: Nil =>
+    }
+  }
+
   private def buildTyper(dynamicPropertyAccessAllowed: Boolean = false) = new Typer(
     dictTyper = new KeysDictTyper(new SimpleDictRegistry(Map.empty)),
     strictMethodsChecking = false,