Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Non-existing context entries result in null #696

Merged
merged 11 commits into from
Sep 5, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -871,17 +871,27 @@ class FeelInterpreter {
case ctx: ValContext =>
EvalContext.wrap(ctx.context, context.valueMapper).variable(key) match {
case _: ValError =>
val detailedMessage = ctx.context.variableProvider.keys match {
case Nil => "The context is empty"
case keys => s"Available keys: ${keys.map("'" + _ + "'").mkString(", ")}"
}
error(
failureType = EvaluationFailureType.NO_CONTEXT_ENTRY_FOUND,
failureMessage = s"context contains no entry with key '$key'"
failureMessage = s"No context entry found with key '$key'. $detailedMessage"
)
ValNull
Comment on lines 878 to +882
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ Off-topic: I was wondering a bit about the usage of the implicit EvalContext and the return value of the error() function. The return value is now ignored because ValNull is returned instead. this works, but the error function is now used only for its 'side effect' (i.e. storing the error as a failure in the implicit evaluation context). It feels easy to use this way, but is this idiomatic Scala? Because this is a side-effect right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid point. 😅 This style is only temporary. After the remaining null-handling issues are resolved, I'll clean up the error part.

case x: Val => x
case _ => error(s"context contains no entry with key '$key'")
}
case ValList(list) => ValList(list map (item => path(item, key)))
case ValNull =>
error(
failureType = EvaluationFailureType.NO_CONTEXT_ENTRY_FOUND,
failureMessage = s"No context entry found with key '$key'. The context is null"
)
ValNull
case value =>
value.property(key).getOrElse {
val propertyNames: String = value.propertyNames().mkString(",")
val propertyNames: String = value.propertyNames().map("'" + _ + "'").mkString(", ")
error(
failureType = EvaluationFailureType.NO_PROPERTY_FOUND,
failureMessage = s"No property found with name '$key' of value '$value'. Available properties: $propertyNames"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,13 @@ trait EvaluationResultMatchers {

def returnNull() = new EvaluationResultValueMatcher(expectedResult = null)

def failWith(expectedFailureMessage: String) =
new EvaluationResultFailureMatcher(expectedFailureMessage)

def failToParse() = new EvaluationResultFailureMatcher(
expectedFailureMessage = "failed to parse"
)

def reportFailure(failureType: EvaluationFailureType, failureMessage: String) =
new SuppressedFailureMatcher(EvaluationFailure(failureType, failureMessage))

Expand Down Expand Up @@ -59,6 +66,23 @@ trait EvaluationResultMatchers {
}
}

class EvaluationResultFailureMatcher(expectedFailureMessage: String) extends Matcher[EvaluationResult] {
override def apply(evaluationResult: EvaluationResult): MatchResult = {
evaluationResult match {
case SuccessfulEvaluationResult(result, _) => MatchResult(
false,
s"the evaluation didn't fail with '$expectedFailureMessage' but returned '${result}'",
s"the evaluation didn't fail with '$expectedFailureMessage' but returned '${result}'"
)
case FailedEvaluationResult(failure, _) => MatchResult(
failure.message.contains(expectedFailureMessage),
s"the evaluation failure message didn't contain '$expectedFailureMessage' but was '${failure.message}'",
s"the evaluation failure message contained '${failure.message}' as expected",
)
}
}
}

}

object EvaluationResultMatchers extends EvaluationResultMatchers
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,14 @@ class SuppressedFailuresTest extends AnyFlatSpec
it should "report a suppressed failure for a non-existing context entry" in {
evaluateExpression("{x: 1}.y") should reportFailure(
failureType = EvaluationFailureType.NO_CONTEXT_ENTRY_FOUND,
failureMessage = "context contains no entry with key 'y'"
failureMessage = "No context entry found with key 'y'. Available keys: 'x'"
)
}

it should "report a suppressed failure for a non-existing property" in {
evaluateExpression(""" @"P1Y".days """) should reportFailure(
failureType = EvaluationFailureType.NO_PROPERTY_FOUND,
failureMessage = "No property found with name 'days' of value 'P1Y'. Available properties: years,months"
failureMessage = "No property found with name 'days' of value 'P1Y'. Available properties: 'years', 'months'"
)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,12 @@ class BuiltinFunctionsTest
eval(""" is defined( {"a":1}.a ) """) should be(ValBoolean(true))
}

it should "return false if the value is not present" in {

it should "return false if a variable doesn't exist" in {
eval("is defined(a)") should be(ValBoolean(false))
eval("is defined(a.b)") should be(ValBoolean(false))
}

ignore should "return false if a context entry doesn't exist" in {
eval("is defined({}.a)") should be(ValBoolean(false))
eval("is defined({}.a.b)") should be(ValBoolean(false))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@
*/
package org.camunda.feel.impl.interpreter

import org.camunda.feel.impl.FeelIntegrationTest
import org.camunda.feel.syntaxtree._
import org.camunda.feel.api.EvaluationFailureType
import org.camunda.feel.impl.{EvaluationResultMatchers, FeelEngineTest}
import org.scalatest.flatspec.AnyFlatSpec
import org.scalatest.matchers.should.Matchers

Expand All @@ -27,106 +27,141 @@ import org.scalatest.matchers.should.Matchers
class InterpreterBeanExpressionTest
extends AnyFlatSpec
with Matchers
with FeelIntegrationTest {
with FeelEngineTest
with EvaluationResultMatchers {

"A bean" should "access a field" in {

class A(val b: Int)

eval("a.b", Map("a" -> new A(2))) should be(ValNumber(2))

evaluateExpression(
expression = "a.b",
variables = Map("a" -> new A(2))
) should returnResult(2)
}

it should "access a getter method as field" in {

class A(b: Int) {
def getFoo() = b + 1
}

eval("a.foo", Map("a" -> new A(2))) should be(ValNumber(3))

evaluateExpression(
expression = "a.foo",
variables = Map("a" -> new A(2))
) should returnResult(3)
}

it should "ignore getter method with arguments as field" in {

class A(x: Int) {
def getResult(y: Int): Int = x + y
}

eval("a.result", Map("a" -> new A(2))) should be(
ValError("context contains no entry with key 'result'")
)
evaluateExpression(
expression = "a.result",
variables = Map("a" -> new A(2))
) should (returnNull() and
reportFailure(
failureType = EvaluationFailureType.NO_CONTEXT_ENTRY_FOUND,
failureMessage = "No context entry found with key 'result'. Available keys: "
))
}

it should "ignore method with arguments as field (builder-style)" in {

class A(x: Int) {
def plus(y: Int): A = new A(x + y)
}

eval("a.plus", Map("a" -> new A(2))) should be(
ValError("context contains no entry with key 'plus'")
)
evaluateExpression(
expression = "a.plus",
variables = Map("a" -> new A(2))
) should (returnNull() and
reportFailure(
failureType = EvaluationFailureType.NO_CONTEXT_ENTRY_FOUND,
failureMessage = "No context entry found with key 'plus'. Available keys: "
))
}

it should "not access a private field" in {
class A(private val x: Int)

eval("a.x", Map("a" -> new A(2))) should be(
ValError("context contains no entry with key 'x'")
)
evaluateExpression(
expression = "a.x",
variables = Map("a" -> new A(2))
) should (returnNull() and
reportFailure(
failureType = EvaluationFailureType.NO_CONTEXT_ENTRY_FOUND,
failureMessage = "No context entry found with key 'x'. Available keys: "
))
}

it should "not access a private method" in {
class A(val x: Int) {
private def getResult(): Int = x
}

eval("a.result", Map("a" -> new A(2))) should be(
ValError("context contains no entry with key 'result'")
)
evaluateExpression(
expression = "a.result",
variables = Map("a" -> new A(2))
) should (returnNull() and
reportFailure(
failureType = EvaluationFailureType.NO_CONTEXT_ENTRY_FOUND,
failureMessage = "No context entry found with key 'result'. Available keys: 'x'"
))
}

it should "invoke a method without arguments" in {

class A {
def foo() = "foo"
}

eval("a.foo()", Map("a" -> new A())) should be(ValString("foo"))

evaluateExpression(
expression = "a.foo()",
variables = Map("a" -> new A())
) should returnResult("foo")
}

it should "invoke a method with one argument" in {

class A {
def incr(x: Int) = x + 1
}

eval("a.incr(1)", Map("a" -> new A())) should be(ValNumber(2))

evaluateExpression(
expression = "a.incr(1)",
variables = Map("a" -> new A())
) should returnResult(2)
}

it should "access a nullable field" in {

class A(val a: String, val b: String)

eval(""" a.a = null """, Map("a" -> new A("not null", null))) should be(
ValBoolean(false))
eval(""" a.b = null """, Map("a" -> new A("not null", null))) should be(
ValBoolean(true))
eval(""" null = a.a """, Map("a" -> new A("not null", null))) should be(
ValBoolean(false))
eval(""" null = a.b""", Map("a" -> new A("not null", null))) should be(
ValBoolean(true))
eval(""" a.a = a.b """, Map("a" -> new A("not null", "not null"))) should be(
ValBoolean(true))
eval(""" a.a = a.b """, Map("a" -> new A("not null", null))) should be(
ValBoolean(false))
eval(""" a.a = a.b """, Map("a" -> new A(null, "not null"))) should be(
ValBoolean(false))
eval(""" a.a = a.b """, Map("a" -> new A(null, null))) should be(
ValBoolean(true))
evaluateExpression(
expression = "a.a",
variables = Map("a" -> new A(a = "not null", b = null))
) should returnResult("not null")

evaluateExpression(
expression = "a.b",
variables = Map("a" -> new A(a = "not null", b = null))
) should returnNull()

evaluateExpression(
expression = "a.a = a.b",
variables = Map("a" -> new A(a = "not null", b = "not null"))
) should returnResult(true)

evaluateExpression(
expression = "a.a = a.b",
variables = Map("a" -> new A(a = "not null", b = null))
) should returnResult(false)

evaluateExpression(
expression = "a.a = a.b",
variables = Map("a" -> new A(a = null, b = "not null"))
) should returnResult(false)

evaluateExpression(
expression = "a.a = a.b",
variables = Map("a" -> new A(a = null, b = null))
) should returnResult(true)
}

}