From 4491e20e9136cfdcc41e43a4445b381e2c774fdd Mon Sep 17 00:00:00 2001 From: Pawel Czajka Date: Fri, 13 Dec 2024 11:03:18 +0100 Subject: [PATCH 1/5] fixed issue with toMap on list --- .../engine/extension/ToMapConversionExt.scala | 4 ++++ .../nussknacker/engine/spel/SpelExpressionSpec.scala | 12 ++++++++++++ 2 files changed, 16 insertions(+) diff --git a/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/extension/ToMapConversionExt.scala b/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/extension/ToMapConversionExt.scala index b3bd7abd35f..fa4e333f444 100644 --- a/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/extension/ToMapConversionExt.scala +++ b/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/extension/ToMapConversionExt.scala @@ -83,6 +83,10 @@ object ToMapConversion extends Conversion[JMap[_, _]] { val map = new JHashMap[Any, Any]() c.forEach(e => map.put(e.get(keyName), e.get(valueName))) Right(map) + case c: JCollection[java.util.Map.Entry[_, _] @unchecked] => + val map = new JHashMap[Any, Any]() + c.forEach(e => map.put(e.getKey, e.getValue)) + Right(map) case x => Left(new IllegalArgumentException(s"Cannot convert: $x to a Map")) } diff --git a/scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSpec.scala b/scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSpec.scala index 91cdd3b104b..b9ffab153eb 100644 --- a/scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSpec.scala +++ b/scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSpec.scala @@ -1663,6 +1663,18 @@ class SpelExpressionSpec extends AnyFunSuite with Matchers with ValidatedValuesD } } + test("should convert list of map entries into map") { + // this is a special case + // list on which we run .toMap contains elements of type java.util.Map.Entry + val result = evaluate[Any](""" + |{ + |a: "A", + |b: "B" + |}.![#this].toMap + |""".stripMargin) + result shouldBe java.util.Map.of("a", "A", "b", "B") + } + test("should return error msg if record in map project does not contain required fields") { parse[Any]("#mapValue.![{invalid_key: #this.key}].toMap()", ctx).invalidValue.toList should matchPattern { case GenericFunctionError("List element must contain 'key' and 'value' fields") :: Nil => From 4a7828ba8f3ab91f3cb50261f482dd1004c02021 Mon Sep 17 00:00:00 2001 From: Pawel Czajka Date: Fri, 13 Dec 2024 12:15:00 +0100 Subject: [PATCH 2/5] avoid type erasure issue, also make it work with list of mixed entries --- .../engine/extension/ToMapConversionExt.scala | 31 ++++++++++--------- .../engine/spel/SpelExpressionSpec.scala | 24 ++++++++++++++ 2 files changed, 41 insertions(+), 14 deletions(-) diff --git a/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/extension/ToMapConversionExt.scala b/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/extension/ToMapConversionExt.scala index fa4e333f444..413b221c910 100644 --- a/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/extension/ToMapConversionExt.scala +++ b/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/extension/ToMapConversionExt.scala @@ -77,17 +77,10 @@ object ToMapConversion extends Conversion[JMap[_, _]] { @tailrec override def convertEither(value: Any): Either[Throwable, JMap[_, _]] = value match { - case m: JMap[_, _] => Right(m) - case a: Array[_] => convertEither(ConversionHandler.convertArrayToList(a)) - case c: JCollection[JMap[_, _] @unchecked] if canConvertToMap(c) => - val map = new JHashMap[Any, Any]() - c.forEach(e => map.put(e.get(keyName), e.get(valueName))) - Right(map) - case c: JCollection[java.util.Map.Entry[_, _] @unchecked] => - val map = new JHashMap[Any, Any]() - c.forEach(e => map.put(e.getKey, e.getValue)) - Right(map) - case x => Left(new IllegalArgumentException(s"Cannot convert: $x to a Map")) + case m: JMap[_, _] => Right(m) + case a: Array[_] => convertEither(ConversionHandler.convertArrayToList(a)) + case c: JCollection[_] if canConvertToMap(c) => Right(convertToMap(c)) + case x => Left(new IllegalArgumentException(s"Cannot convert: $x to a Map")) } // We could leave underlying method using convertEither as well but this implementation is faster @@ -98,13 +91,23 @@ object ToMapConversion extends Conversion[JMap[_, _]] { case _ => false } - private def canConvertToMap(c: JCollection[_]): Boolean = c.isEmpty || c + private def canConvertToMap(c: JCollection[_]): Boolean = c .stream() .allMatch { - case m: JMap[_, _] => m.keySet().containsAll(keyAndValueNames) - case _ => false + case m: JMap[_, _] => m.keySet().containsAll(keyAndValueNames) + case _: java.util.Map.Entry[_, _] => true + case _ => false } + private def convertToMap(c: JCollection[_]): JMap[_, _] = { + val map = new JHashMap[Any, Any]() + c.forEach { + case e: JMap[_, _] => map.put(e.get(keyName), e.get(valueName)) + case e: java.util.Map.Entry[_, _] => map.put(e.getKey, e.getValue) + } + map + } + override def appliesToConversion(clazz: Class[_]): Boolean = clazz != resultTypeClass && (clazz.isAOrChildOf(collectionClass) || clazz == unknownClass || clazz.isArray) diff --git a/scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSpec.scala b/scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSpec.scala index b9ffab153eb..7c3ee919a99 100644 --- a/scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSpec.scala +++ b/scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSpec.scala @@ -1675,6 +1675,30 @@ class SpelExpressionSpec extends AnyFunSuite with Matchers with ValidatedValuesD result shouldBe java.util.Map.of("a", "A", "b", "B") } + test("should be able to convert list of map entries into map") { + // this is a special case + // list on which we run .canBeMap contains elements of type java.util.Map.Entry + val result = evaluate[Any](""" + |{ + |a: "A", + |b: "B" + |}.![#this].canBeMap + |""".stripMargin) + result shouldBe true + } + + test("should be able to convert list of map entries into map or null") { + // this is a special case + // list on which we run .toMapOrNull contains elements of type java.util.Map.Entry + val result = evaluate[Any](""" + |{ + |a: "A", + |b: "B" + |}.![#this].toMapOrNull + |""".stripMargin) + result shouldBe java.util.Map.of("a", "A", "b", "B") + } + test("should return error msg if record in map project does not contain required fields") { parse[Any]("#mapValue.![{invalid_key: #this.key}].toMap()", ctx).invalidValue.toList should matchPattern { case GenericFunctionError("List element must contain 'key' and 'value' fields") :: Nil => From a4f178b0fac86136f838427a672cd7f1ac32970c Mon Sep 17 00:00:00 2001 From: Pawel Czajka Date: Fri, 13 Dec 2024 16:37:34 +0100 Subject: [PATCH 3/5] used JMap --- .../nussknacker/engine/extension/ToMapConversionExt.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/extension/ToMapConversionExt.scala b/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/extension/ToMapConversionExt.scala index 413b221c910..e6a11380415 100644 --- a/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/extension/ToMapConversionExt.scala +++ b/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/extension/ToMapConversionExt.scala @@ -95,7 +95,7 @@ object ToMapConversion extends Conversion[JMap[_, _]] { .stream() .allMatch { case m: JMap[_, _] => m.keySet().containsAll(keyAndValueNames) - case _: java.util.Map.Entry[_, _] => true + case _: JMap.Entry[_, _] => true case _ => false } @@ -103,7 +103,7 @@ object ToMapConversion extends Conversion[JMap[_, _]] { val map = new JHashMap[Any, Any]() c.forEach { case e: JMap[_, _] => map.put(e.get(keyName), e.get(valueName)) - case e: java.util.Map.Entry[_, _] => map.put(e.getKey, e.getValue) + case e: JMap.Entry[_, _] => map.put(e.getKey, e.getValue) } map } From fbf281543dda84b18f029d08e90ffc908c7bf8fd Mon Sep 17 00:00:00 2001 From: Pawel Czajka Date: Fri, 13 Dec 2024 16:44:08 +0100 Subject: [PATCH 4/5] removed comments --- .../touk/nussknacker/engine/spel/SpelExpressionSpec.scala | 6 ------ 1 file changed, 6 deletions(-) diff --git a/scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSpec.scala b/scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSpec.scala index 7c3ee919a99..80be87b147d 100644 --- a/scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSpec.scala +++ b/scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSpec.scala @@ -1664,8 +1664,6 @@ class SpelExpressionSpec extends AnyFunSuite with Matchers with ValidatedValuesD } test("should convert list of map entries into map") { - // this is a special case - // list on which we run .toMap contains elements of type java.util.Map.Entry val result = evaluate[Any](""" |{ |a: "A", @@ -1676,8 +1674,6 @@ class SpelExpressionSpec extends AnyFunSuite with Matchers with ValidatedValuesD } test("should be able to convert list of map entries into map") { - // this is a special case - // list on which we run .canBeMap contains elements of type java.util.Map.Entry val result = evaluate[Any](""" |{ |a: "A", @@ -1688,8 +1684,6 @@ class SpelExpressionSpec extends AnyFunSuite with Matchers with ValidatedValuesD } test("should be able to convert list of map entries into map or null") { - // this is a special case - // list on which we run .toMapOrNull contains elements of type java.util.Map.Entry val result = evaluate[Any](""" |{ |a: "A", From b8f4cbe9d71e2068a005707dfa9491a403c8b5c5 Mon Sep 17 00:00:00 2001 From: Pawel Czajka Date: Fri, 13 Dec 2024 16:53:24 +0100 Subject: [PATCH 5/5] handled invalid value case --- .../engine/extension/ToMapConversionExt.scala | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/extension/ToMapConversionExt.scala b/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/extension/ToMapConversionExt.scala index e6a11380415..77ebcb5510a 100644 --- a/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/extension/ToMapConversionExt.scala +++ b/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/extension/ToMapConversionExt.scala @@ -79,10 +79,14 @@ object ToMapConversion extends Conversion[JMap[_, _]] { value match { case m: JMap[_, _] => Right(m) case a: Array[_] => convertEither(ConversionHandler.convertArrayToList(a)) - case c: JCollection[_] if canConvertToMap(c) => Right(convertToMap(c)) - case x => Left(new IllegalArgumentException(s"Cannot convert: $x to a Map")) + case c: JCollection[_] if canConvertToMap(c) => convertToMap(c).map(e => Right(e)).getOrElse(cannotConvertMessage(c)) + case x => cannotConvertMessage(x) } + private def cannotConvertMessage(c: Any): Left[IllegalArgumentException, Nothing] = { + Left(new IllegalArgumentException(s"Cannot convert: $c to a Map")) + } + // We could leave underlying method using convertEither as well but this implementation is faster override def canConvert(value: Any): JBoolean = value match { case _: JMap[_, _] => true @@ -99,13 +103,20 @@ object ToMapConversion extends Conversion[JMap[_, _]] { case _ => false } - private def convertToMap(c: JCollection[_]): JMap[_, _] = { + private def convertToMap(c: JCollection[_]): Option[JMap[_, _]] = { val map = new JHashMap[Any, Any]() + var foundError = false c.forEach { case e: JMap[_, _] => map.put(e.get(keyName), e.get(valueName)) case e: JMap.Entry[_, _] => map.put(e.getKey, e.getValue) + case _ => foundError = true + } + if (foundError) { + // internal error of this class, should not have been called with such value + None + } else { + Some(map) } - map } override def appliesToConversion(clazz: Class[_]): Boolean =