From 25bb5776bdff7679acbb1d755955c926a67c7056 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Bigorajski?= <72501021+lukasz-bigorajski@users.noreply.github.com> Date: Mon, 4 Nov 2024 09:36:54 +0100 Subject: [PATCH] [NU-1836] Handle extension methods in PropertyAccessors (#7118) --- .../engine/extension/ExtensionMethods.scala | 2 + .../touk/nussknacker/engine/spel/Typer.scala | 19 ++++--- .../internal/OptimizedEvaluationContext.scala | 3 +- .../spel/internal/propertyAccessors.scala | 35 ++++++++++++- .../engine/spel/SpelExpressionSpec.scala | 51 +++++++++++-------- .../nussknacker/engine/spel/TyperSpec.scala | 2 +- 6 files changed, 80 insertions(+), 32 deletions(-) diff --git a/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/extension/ExtensionMethods.scala b/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/extension/ExtensionMethods.scala index e2b1ffdfc71..813f2933372 100644 --- a/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/extension/ExtensionMethods.scala +++ b/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/extension/ExtensionMethods.scala @@ -17,6 +17,8 @@ class ExtensionsAwareMethodInvoker(classDefinitionSet: ClassDefinitionSet) { toInvocationTargetConvertersByClass .get(method.getDeclaringClass) .map(_.toInvocationTarget(target)) + // Maybe in future we could write some mechanism to invoke extension methods statically. What I mean is to + // find correct extension based on target and then implement simple switch to fire method based on name .map(impl => method.invoke(impl, arguments: _*)) .getOrElse { throw new IllegalArgumentException(s"Extension method: ${method.getName} is not implemented") diff --git a/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/Typer.scala b/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/Typer.scala index 9f4c36ae0f8..099e5c44183 100644 --- a/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/Typer.scala +++ b/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/Typer.scala @@ -64,7 +64,7 @@ private[spel] class Typer( staticMethodInvocationsChecking: Boolean, classDefinitionSet: ClassDefinitionSet, evaluationContextPreparer: EvaluationContextPreparer, - methodExecutionForUnknownAllowed: Boolean, + anyMethodExecutionForUnknownAllowed: Boolean, dynamicPropertyAccessAllowed: Boolean ) extends LazyLogging { @@ -73,7 +73,7 @@ private[spel] class Typer( private lazy val evaluationContext: EvaluationContext = evaluationContextPreparer.prepareEvaluationContext(Context(""), Map.empty) - private val methodReferenceTyper = new MethodReferenceTyper(classDefinitionSet, methodExecutionForUnknownAllowed) + private val methodReferenceTyper = new MethodReferenceTyper(classDefinitionSet, anyMethodExecutionForUnknownAllowed) private lazy val typeReferenceTyper = new TypeReferenceTyper(evaluationContext, classDefinitionSet) @@ -607,10 +607,14 @@ private[spel] class Typer( private def extractProperty(e: PropertyOrFieldReference, t: TypingResult): TypingR[TypingResult] = t match { case Unknown => val w = Writer.value[List[ExpressionParseError], TypingResult](Unknown) - if (methodExecutionForUnknownAllowed) + if (anyMethodExecutionForUnknownAllowed) { w - else - w.tell(List(IllegalPropertyAccessError(Unknown))) + } else { + // we allow some methods to be used on unknown + unknownPropertyTypeBasedOnMethod(e) + .map(valid) + .getOrElse(w.tell(List(IllegalPropertyAccessError(Unknown)))) + } case TypedNull => invalid(IllegalPropertyAccessError(TypedNull), fallbackType = TypedNull) case s: SingleTypingResult => @@ -681,6 +685,9 @@ private[spel] class Typer( classDefinitionSet.get(clazz.klass).flatMap(_.getPropertyOrFieldType(invocationTarget, e.getName)) } + private def unknownPropertyTypeBasedOnMethod(e: PropertyOrFieldReference): Option[TypingResult] = + classDefinitionSet.unknown.flatMap(_.getPropertyOrFieldType(Unknown, e.getName)) + private def extractIterativeType(parent: TypingResult): TypingR[TypingResult] = parent match { case tc: SingleTypingResult if tc.runtimeObjType.canBeSubclassOf(Typed[java.util.Collection[_]]) || @@ -762,7 +769,7 @@ private[spel] class Typer( staticMethodInvocationsChecking, classDefinitionSet, evaluationContextPreparer, - methodExecutionForUnknownAllowed, + anyMethodExecutionForUnknownAllowed, dynamicPropertyAccessAllowed ) diff --git a/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/internal/OptimizedEvaluationContext.scala b/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/internal/OptimizedEvaluationContext.scala index b445d2dcc43..1c1cecaeb0f 100644 --- a/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/internal/OptimizedEvaluationContext.scala +++ b/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/internal/OptimizedEvaluationContext.scala @@ -96,7 +96,8 @@ object EvaluationContextPreparer { val conversionService = determineConversionService(expressionConfig) val propertyAccessors = internal.propertyAccessors.configured( - MethodAccessChecker.create(classDefinitionSet, expressionConfig.dynamicPropertyAccessAllowed) + MethodAccessChecker.create(classDefinitionSet, expressionConfig.dynamicPropertyAccessAllowed), + classDefinitionSet, ) new EvaluationContextPreparer( classLoader, diff --git a/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/internal/propertyAccessors.scala b/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/internal/propertyAccessors.scala index d321c886670..8a32f6c4524 100644 --- a/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/internal/propertyAccessors.scala +++ b/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/internal/propertyAccessors.scala @@ -6,6 +6,7 @@ import org.springframework.util.ClassUtils import pl.touk.nussknacker.engine.api.dict.DictInstance import pl.touk.nussknacker.engine.api.exception.NonTransientException import pl.touk.nussknacker.engine.definition.clazz.ClassDefinitionSet +import pl.touk.nussknacker.engine.extension.{ExtensionAwareMethodsDiscovery, ExtensionsAwareMethodInvoker} import java.lang.reflect.{Field, Method, Modifier} import java.util.Optional @@ -17,7 +18,8 @@ object propertyAccessors { // This general order can be overridden - each accessor can define target classes for which it will have precedence - // through the `getSpecificTargetClasses` method. def configured( - accessChecker: MethodAccessChecker + accessChecker: MethodAccessChecker, + set: ClassDefinitionSet ): Seq[PropertyAccessor] = { Seq( MapPropertyAccessor, // must be before NoParamMethodPropertyAccessor and ReflectivePropertyAccessor @@ -29,9 +31,10 @@ object propertyAccessors { new StaticPropertyAccessor(accessChecker), TypedDictInstancePropertyAccessor, // must be before NoParamMethodPropertyAccessor new NoParamMethodPropertyAccessor(accessChecker), + new ExtensionMethodsPropertyAccessor(accessChecker, set), // it can add performance overhead so it will be better to keep it on the bottom MapLikePropertyAccessor, - MapMissingPropertyToNullAccessor, // must be after NoParamMethodPropertyAccessor + MapMissingPropertyToNullAccessor, // must be after NoParamMethodPropertyAccessor, ExtensionMethodsPropertyAccessor ) } @@ -304,6 +307,34 @@ object propertyAccessors { override def getSpecificTargetClasses: Array[Class[_]] = null } + // It's not optimized to generate byteCode like in ReflectivePropertyAccessor so it's running only in interpreted mode + private class ExtensionMethodsPropertyAccessor( + accessChecker: MethodAccessChecker, + set: ClassDefinitionSet + ) extends PropertyAccessor + with ReadOnly + with Caching { + private val methodInvoker = new ExtensionsAwareMethodInvoker(set) + private val emptyArray = Array[AnyRef]() + + override def getSpecificTargetClasses: Array[Class[_]] = null + + override protected def invokeMethod( + propertyName: String, + method: Method, + target: Any, + context: EvaluationContext + ): Any = methodInvoker.invoke(method)(target, emptyArray) + + override protected def reallyFindMethod(name: String, target: Class[_]): Option[Method] = + accessChecker.checkAccessIfMethodFound(target) { + ExtensionAwareMethodsDiscovery + .discover(target) + .find(m => m.getParameterCount == 0 && m.getName == name) + } + + } + trait Caching extends CachingBase { self: PropertyAccessor => override def canRead(context: EvaluationContext, target: scala.Any, name: String): Boolean = 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 02679098923..7bb42c2a885 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 @@ -1618,17 +1618,17 @@ class SpelExpressionSpec extends AnyFunSuite with Matchers with ValidatedValuesD Table( ("expression", "expectedType", "expectedResult"), ( - "#stringMap.![{key: #this.key + '_k', value: #this.value + '_v'}].toMap()", + "#stringMap.![{key: #this.key + '_k', value: #this.value + '_v'}].toMap", mapStringStringType, Map("foo_k" -> "bar_v", "baz_k" -> "qux_v").asJava ), ( - "#mapWithDifferentValueTypes.![{key: #this.key, value: #this.value}].toMap()", + "#mapWithDifferentValueTypes.![{key: #this.key, value: #this.value}].toMap", mapStringUnknownType, mapWithDifferentValueTypes ), ( - "#nullableMap.![{key: #this.key, value: #this.value}].toMap()", + "#nullableMap.![{key: #this.key, value: #this.value}].toMap", mapStringStringType, nullableMap ) @@ -1872,25 +1872,25 @@ class SpelExpressionSpec extends AnyFunSuite with Matchers with ValidatedValuesD ("#unknownMap.value.is('Map')", true), ("#unknownListOfTuples.value.is('List')", true), ("#unknownListOfTuples.value.is('Map')", true), - ("#unknownBoolean.value.isBoolean()", true), - ("#unknownBooleanString.value.isBoolean()", true), - ("#unknownString.value.isBoolean()", false), - ("#unknownLong.value.isLong()", true), - ("#unknownLongString.value.isLong()", true), - ("#unknownString.value.isLong()", false), - ("#unknownDouble.value.isDouble()", true), - ("#unknownDoubleString.value.isDouble()", true), - ("#unknownString.value.isDouble()", false), - ("#unknownBigDecimal.value.isBigDecimal()", true), - ("#unknownBigDecimalString.value.isBigDecimal()", true), - ("#unknownString.value.isBigDecimal()", false), - ("#unknownList.value.isList()", true), - ("#unknownList.value.isMap()", false), - ("#unknownMap.value.isList()", false), - ("#unknownMap.value.isMap()", true), - ("#unknownListOfTuples.value.isList()", true), - ("#unknownListOfTuples.value.isMap()", true), - ("#arrayOfUnknown.isList()", true), + ("#unknownBoolean.value.isBoolean", true), + ("#unknownBooleanString.value.isBoolean", true), + ("#unknownString.value.isBoolean", false), + ("#unknownLong.value.isLong", true), + ("#unknownLongString.value.isLong", true), + ("#unknownString.value.isLong", false), + ("#unknownDouble.value.isDouble", true), + ("#unknownDoubleString.value.isDouble", true), + ("#unknownString.value.isDouble", false), + ("#unknownBigDecimal.value.isBigDecimal", true), + ("#unknownBigDecimalString.value.isBigDecimal", true), + ("#unknownString.value.isBigDecimal", false), + ("#unknownList.value.isList", true), + ("#unknownList.value.isMap", false), + ("#unknownMap.value.isList", false), + ("#unknownMap.value.isMap", true), + ("#unknownListOfTuples.value.isList", true), + ("#unknownListOfTuples.value.isMap", true), + ("#arrayOfUnknown.isList", true), ) ) { (expression, result) => evaluate[Any](expression, customCtx) shouldBe result @@ -1933,6 +1933,13 @@ class SpelExpressionSpec extends AnyFunSuite with Matchers with ValidatedValuesD parsed.evaluateSync[Any](ctx) shouldBe List("a").asJava } + test("should allow use no param method property accessor on unknown") { + val customCtx = ctx.withVariable("unknownInt", ContainerOfUnknown("11")) + val parsed = parse[Any]("#unknownInt.value.toLongOrNull", customCtx).validValue + parsed.evaluateSync[Any](customCtx) shouldBe 11 + parsed.evaluateSync[Any](customCtx) shouldBe 11 + } + } case class SampleObject(list: java.util.List[SampleValue]) diff --git a/scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/TyperSpec.scala b/scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/TyperSpec.scala index a87f3b2826b..9edd8839cc4 100644 --- a/scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/TyperSpec.scala +++ b/scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/TyperSpec.scala @@ -182,7 +182,7 @@ class TyperSpec extends AnyFunSuite with Matchers with ValidatedValuesDetailedMe staticMethodInvocationsChecking = false, classDefinitionSet = ClassDefinitionTestUtils.createDefinitionWithDefaultsAndExtensions, evaluationContextPreparer = null, - methodExecutionForUnknownAllowed = false, + anyMethodExecutionForUnknownAllowed = false, dynamicPropertyAccessAllowed = dynamicPropertyAccessAllowed )