Misleading useNull flag

[engi2nee]

In the InstantiatorConfig we can pass useNull as false, which I expected to fill all nullables with random values.

Then I noticed In the Readme: If config.useNull = false then InstanceFactor will be called to decide if a null or non-null value is returned.

digging into the code base, I found toNullableInstanceFactory which is set to use ToNullableInstanceFactoryMode.RANDOM (Randomly return null or a Random value) in all default nullable factories inside DEFAULT_INSTANCE_FACTORIES, with no way to change it except for new custom factories.

I think useNull boolean flag should be replaced by a ToNullableInstanceFactoryMode check, as there is no way to make default nullable factories never return null.

[sockeqwe]

Hello, yes, I agree it is a bit confusing.

My reasoning / how it works is:

if you use config.useNull = true then every constructor parameter that can be null will be filled automatically with null values without even asking an InstanceFactory

Example:

data class MyClass(val name : String?)

val config = InstantiatorConfig(useNull = true)

val x = instance<MyClass>(config)
println(x.name) // always prints null

if you use

data class MyClass(val name : String?)

val config = InstantiatorConfig(useNull = false)

val x = instance<MyClass>(config)
println(x.name) // could be null or not null. It depends on the StringInstanceFactory which uses random

So useNull takes precedence before InstanceFactory will be asked.

I hope it is more understandable now.

With that said, I agree that it is confusing. What do you think about renaming it touseNullOnOptionalParameters? Would that make things more clear or do you think it needs an entirely different approach?

[engi2nee]

Thanks for the quick reply, The issue isn't only about the naming of the flag, An example would be a network entity, with lots of nullable fields.

// Entities
data class UserEntity(
    val id: String,
    val firstName: String?,
    val lastName: String?,
    val createdOn: Long?,
    val status: String?,
    val car: CarEntity?,
    ...
)

data class CarEntity(val model: String, val color: String)

// Domain
data class UserCar(val model: String, val color: String)

object Mapper {
    fun entityToDomain(entity: UserEntity): UserCar = UserCar(
        model = entity.car?.model ?: "INVISIBLE CAR",
        color = entity.car?.color ?: "Transparent"
    )
}

// Test cases
@Test
fun testNullCar() {
    val entity = instance<UserEntity>(InstantiatorConfig(useNull = true))
    val expected = Mapper.entityToDomain(entity)

    assertEquals(expected.model, "INVISIBLE CAR")
    assertEquals(expected.color, "Transparent")
}

@Test
fun testNonNullCarFlaky() {
    // This test case will randomly fail if random.nextBoolean() in toNullableInstanceFactory returned false
   // Because expected.model will be "INVISIBLE CAR" while entity.car.model is null

    val entity = instance<UserEntity>(InstantiatorConfig(useNull = false))
    val expected = Mapper.entityToDomain(entity)

    assertEquals(expected.model, entity.car?.model)
    assertEquals(expected.color, entity.car?.color)
}

@Test
fun testNonNullCar() {
    // This will work, but defeats the purpose of the library, especially on bigger examples
    val entity = instance<UserEntity>().copy(car = CarEntity(model = getRandomString(), color = getRandomString()))
    val expected = Mapper.entityToDomain(entity)

    assertEquals(expected.model, entity.car?.model)
    assertEquals(expected.color, entity.car?.color)
}

Ofcouse we can always create a custom factory for such cases, but again doing so for every big data class is too much. Another option would be copying all default factories and recreating their nullable version with toNullableInstanceFactory(mode = NEVER_NULL ), which I think isn't a good idea anyway.

I suggest to remove useNull fully, and replace it with something like NullabilityMode where user can select the mode between RANDOM, ALWAYS_NULL, NEVER_NULL.

And when set to ALWAYS_NULL, then we can skip asking InstanceFactory just like we have currently.

[sockeqwe]

Yes, I understand what you mean. I think something like NullabilityMode could help and make things explicit. For ALWAYS_NULL and NEVER_NULL things are clear. NullabilityMode.RANDOM could be a bit tricky and maybe needs a bit more thought and how this could work with InstanceFactories.

In the meantime I think something like

val entity = instance<UserEntity>().copy( car = instance<CarEntity>() )   // creates a non-nullable CarEntity

is a valid use case IMHO (but I agree it could be more convenient)

[engi2nee]

That works but with 2 downsides: 1- intellij idea linter will warn that <CarEntity> is not needed (explicit type arguments), which can be missed and deleted (or even auto deleted by some linters), causing nullable values to show up again. 2- if the data class has a deep nested nullable field, then it will end up with something like this:

val entity = instance<SomeTypeWithNestedNullables>().copy(
    nonNullableField1 = instance().copy(
        nonNullableField2 = instance().copy(
            nestedNullableField = instance<NonNullType> // just to set this one as non nullable
        )
    ) 

I will see if I can pick this up as well If you are ok with it.

[sockeqwe]

I was mainly pointing out a temporarily solution. I think going with something like NullabilityMode as suggested by you makes sense. I just need to think more in detail through the RANDOM case.

For example: at the moment you can create a https://github.com/sockeqwe/Instantiator/blob/main/src/main/kotlin/com/hannesdorfmann/instantiator/InstantiatorConfig.kt#L280

There is no concept of an enum such as NullabilityMode on InstanceFactory. In particular NullableInstanceFactory can return null or non-null values. Not sure how we could ensure that every NullableInstanceFactory (even a custom one that one developer creates somewhere out there) would take a NullabilityMode.RANDOM successfully into account as intended (without running into nullpointer exceptions internally in Instantiator).

[vadiole]

I've run into this problem as well. It is very confusing, it took me a long time to figure out why I was getting null.

Definitely need to change the api in the future, but for now I will have to use some hack