jackson-module-kotlin icon indicating copy to clipboard operation
jackson-module-kotlin copied to clipboard

CSV EMPTY_STRING_AS_NULL with non-nullable default value = exception

Open hosswald opened this issue 3 years ago • 5 comments

Describe the bug I have a csv with empty cells. I generally want those to be parsed as null. However, I want to also have the ability to parse these cells into non-nullable properties by defining a default value in the constructor. Unfortunately, this leads to an exception:

Instantiation of [simple type, class mypackage.Foo] value failed for JSON property bar due to missing (therefore NULL) value for creator parameter bar which is a non-nullable type
 at [Source: (StringReader); line: 2, column: 2] (through reference chain: mypackage.Foo["bar"])
com.fasterxml.jackson.module.kotlin.MissingKotlinParameterException: Instantiation of [simple type, class mypackage.Foo] value failed for JSON property bar due to missing (therefore NULL) value for creator parameter bar which is a non-nullable type
 at [Source: (StringReader); line: 2, column: 2] (through reference chain: mypackage.Foo["bar"])
	at app//com.fasterxml.jackson.module.kotlin.KotlinValueInstantiator.createFromObjectWith(KotlinValueInstantiator.kt:84)
	at app//com.fasterxml.jackson.databind.deser.impl.PropertyBasedCreator.build(PropertyBasedCreator.java:202)
	at app//com.fasterxml.jackson.databind.deser.BeanDeserializer._deserializeUsingPropertyBased(BeanDeserializer.java:444)
	at app//com.fasterxml.jackson.databind.deser.BeanDeserializerBase.deserializeFromObjectUsingNonDefault(BeanDeserializerBase.java:1405)
	at app//com.fasterxml.jackson.databind.deser.BeanDeserializer.deserializeFromObject(BeanDeserializer.java:352)
	at app//com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:185)
	at app//com.fasterxml.jackson.databind.MappingIterator.nextValue(MappingIterator.java:283)
	at app//com.fasterxml.jackson.databind.MappingIterator.readAll(MappingIterator.java:323)
	at app//com.fasterxml.jackson.databind.MappingIterator.readAll(MappingIterator.java:309)

To Reproduce

data class Foo(val bar: String = "default", val baz: String)

class EmptyStringAsNullIntoNunNullableParameterWithDefaultValue {
    @Test
    internal fun test() {
        val parsed = CsvMapper()
            .registerModule(KotlinModule())
            .readerFor(Foo::class.java)
            .withFeatures(CsvParser.Feature.EMPTY_STRING_AS_NULL)
            .with(CsvSchema.emptySchema().withHeader())
            .readValues<Foo>(
                """
                bar,    baz
                ,       42                
            """.trimIndent()
            ).readAll()
        println(parsed)
    }
}

Expected behavior It can be argued that a json like {"bar":null, "baz":"42"} should produce an error when parsed into Foo, because there is an explicit null.

However, in the case of CSV, there is no such thing as "explicit null". The value is only null because of CsvParser.Feature.EMPTY_STRING_AS_NULL. Therefore, I would expect the above test not to fail. KotlinValueInstantiator should probably handle this case by setting isMissing = true, which would likely fix the problem (I did not test this, I only had a brief look at the code).

Versions Kotlin: 1.7.20 Jackson-module-kotlin: 2.14.0 Jackson-databind: 2.14.0

hosswald avatar Nov 16 '22 15:11 hosswald

Ok, this fixes it for me:

KotlinModule.Builder()
    .configure(KotlinFeature.NullIsSameAsDefault, true)
.build()

However, I'm not really passing null here, I'm just deciding how empty cells in my CSV should be handled (absent any other value, such as a default value), so it feels a bit odd that I have to enable this KotlinFeature.

hosswald avatar Nov 16 '22 16:11 hosswald

Put differently, the usage of default parameter should be the default when parsing a CSV and encountering an empty cell, regardless of whether or not EMPTY_STRING_AS_NULL is used, in my opinion. default by default, so to say.

hosswald avatar Nov 16 '22 16:11 hosswald

@hosswald Unfortunately I am not sure it is possible to have such cross-dependencies; use of CSV backend is independent of use of Kotlin module. And from architecture perspective there is no good way to add such constraints, even if they might make sense.

cowtowncoder avatar Nov 17 '22 03:11 cowtowncoder

@cowtowncoder I just looked a bit through the test cases for jackson-dataformats-text/csv... MissingColumnsTest::testDefaultMissingHandling() handles missing columns the way I would like missing cells (in a present column) to be handled, but NullReadTest::testEmptyStringAsNull330() ensures empty cells are handled as null even if there is a default value. Do you think it makes sense to start a discussion in the issuer tracker there?

hosswald avatar Nov 17 '22 08:11 hosswald

@hosswald Yes, I think that if handling can be defined in more general way, not relying on some specific Kotlin aspect, it'd make sense to file an RFE there.

cowtowncoder avatar Nov 17 '22 17:11 cowtowncoder