mapVariable allows null values when value type is not nullable

[rvinzent]

Steps to reproduce

• camunda-bpm-data version: 1.2.3
• Camunda BPM version: 7.16.0-ee
• JDK version: 11 (Kotlin 1.5.20)
• Operating system: MacOs
• Complete executable reproducer: (e.g. GitHub Repo): the problem is simple enough...
• Steps: (what exactly are you doing with the above reproducer?)
  • run the below test

object TestVariables {
    val noNullValues: VariableFactory<Map<String, Any>> = mapVariable("noNullValues")
}

class TestIssue {
    @Test
    fun `test null values throw WrongVariableTypeException` {
        val data = mapOf("first" to 1, "second" to null)
        val mockExecution = delegateExecutionFake().withVariable("noNullValues", data)
        assertThrows<WrongVariableTypeException> {
            TestVariables.notNullValues.from(mockExecution).get()
        }
    }
}

Expected behaviour

• expect WrongVariableTypeException to be thrown
• using a Kotlin extension leads me to believe the types of Map keys and values is correct, even in Kotlin

Actual behaviour

• null value is allowed, violating Kotlin's null safety guarantees

---

I cannot work around this problem with a nullable type because type bounds of mapVariable don't allow a nullable type, so changing my value type to Any? is not allowed by the compiler.

inline fun <reified K : Any, reified V : Any> mapVariable(name: String): VariableFactory<Map<K, V>> =
    MapVariableFactory(name, K::class.java, V::class.java)

This issue makes using map variables dangerous as the values are all implicitly nullable even though I can't use an appropriate nullable type. Even knowing this, the IDE still throws warnings everywhere that I am checking for null on what should be a non-nullable value.

[zambrovski]

Hi Ryan,

Thank you for your question.

Indeed, this is a serious issue. I was already thinking of how to work around this for a while. The problem here is not the Kotlin side but the Camunda API.

In fact your are faking around the library. Using delegateExecutionFake().withVariable(NO_NULL_MAP.name, data) you are bypassing the camunda-bpm-data library and use Camunda API (implemented in Java having a weaker type system) and put wrong values into the execution. Then you are trying to read it using the Kotlin (strong) type system...

If you try to use the library in both cases, the problem will disappear, since there compiler will not let you do this:

    val data = mapOf("first" to 1, "second" to null)
    val mockExecution = delegateExecutionFake()
    NO_NULL_MAP.on(mockExecution).set(data) // compile error, data is Map<String, Any?> and is not the expected type Map<String, Any>
    assertThrows(WrongVariableTypeException::class.java) {
      NO_NULL_MAP.from(mockExecution).get()
    }

What I think of is to provide an additional mapVariableNullable allowing to pass nullable maps inside and retrieve the nullable maps back... Will it help?

But you are free to use it in your project if you need this:

inline fun <reified K : Any, reified V : Any?> mapVariableNullable(name: String): VariableFactory<Map<K, V>> = MapVariableFactory(name, K::class.java, V::class.java)

I think you initial issue is hard to detect and almost impossible to implement. You are defining the variable to be type of Map<String, Any> and its content passed through Java and Kotlin (type information is erased)... On access to second in the map you will probably get a Kotlin NullPointerException, since the type of the value is not matching its definition (actually impossible condition from the Kotlin point of view).

What do you think?

Cheers,

Simon

[rvinzent]

@zambrovski Thanks for the response! Good to know it's on your radar and your explanation sounds reasonable. In this case, we are expecting this variable to be set by authors of BPMN processes (i.e. not via the library). Since null values are always going to be possible, I would suggest adjusting the generic type bounds on the regular mapVariable to <K : Any, V : Any?>.

I don't believe this would be a breaking change, and anyone using it can expect the same behavior as before because all of the existing Map<K : Any, V : Any> types should be assignable to Map<K : Any, V : Any?>. All the behavior and type safety that was possible before should still function exactly the same, but also allow for proper typing of maps where the developer already knows the values might be null.

Thanks for the workaround, but IMO that signature is a more correct and flexible typing of the existing mapVariable anyway since it better reflects the actual behavior. Can you think of any downsides to the V : Any -> V : Any? change?

[zambrovski]

I'll change the signatures (for Map, List and Set) but will still provide the versions of non-nullable collections.