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

coerceInputValues=true and explicitNulls=false should decode unknown enum value without default to null #2586

Closed
carstenhag opened this issue Mar 1, 2024 · 11 comments
Assignees
Labels

Comments

@carstenhag
Copy link

carstenhag commented Mar 1, 2024

Describe the bug
We updated from 1.6.2 to 1.6.3 and now nullable enums without explicit null default don't get decoded anymore, leading to an exception.

To Reproduce

val json = Json {
    ignoreUnknownKeys = true
    isLenient = true
    coerceInputValues = true
    explicitNulls = false
}

@Serializable
data class Outer(
    @SerialName("service")
    val service: Service?,
)

@Serializable
enum class Service {
    @SerialName("card")
    CARD,
}

val serviceJson = """{"service":"test"}"""
json.decodeFromString<Outer>(serviceJson)

SerializationException: org.example.Service does not contain element with name 'test' at path $.service

Expected behavior
We were expecting that these configs (taken from the documentation) get "added up" together:

  • coerceInputValues = true:
    • Enables coercing incorrect JSON values to the default property value in the following cases:
    • Property type is an enum type, but JSON value contains unknown enum member
  • explicitNulls = false:
    • When this flag is disabled [...] during decoding, the absence of a field value is treated as null for nullable properties without a default value.

In other words: For a nullable enum without default value, set its value to "null" if the JSON value is an unknown enum member.

Other
Apart from combining these two config properties, I wouldn't know how we can get automatic nulls? We would have to set an explicit null to each nullable enum?
Also see #2585.

Environment

  • Kotlin version: 1.9.22
  • Library version: Bug introduced in 1.6.3, working in 1.6.2
  • Kotlin platforms: JVM
  • Gradle version: 8.6
  • Other relevant context: OpenJDK 21.0.2
@sandwwraith
Copy link
Member

Hi! I've reproduced your issue, and it seems there is some misunderstanding because the documentation wasn't formulated clearly enough. It says, Enables coercing incorrect JSON values to the default property value in the following cases:... implying that this configuration flag should only affect properties with default values. However, it worked together with explicitNulls in 1.6.2 because there wasn't a strict check for that. By coincidence, an incorrect enum value was treated as an absence of value — the behavior you describe here was implementation-defined, and we didn't intend to have it when the feature was initially designed (consequently, there aren't any tests for that).
This is also the same reason #2529 was reported — the lack of optionality check led to an incorrect error message about the absence of value instead of reporting the wrong enum constant. This issue was fixed in the 1.6.3 release by introducing this check, and therefore it no longer works with explicitNulls.

To solve this problem right now, I'd recommend adding the default null value to val service: Service?.
However, your use case seems reasonable, so we may consider adding it to documented ones and providing such a feature in future releases.

@carstenhag
Copy link
Author

Technically there is a difference between a value set to null and the value being nullable (without null default), but is this a concern to the library in this case?

Can't it always be assumed that when it's nullable, it should default to null? As when you mark something nullable, you are fully aware it can be missing and that nulls could be present.


If this could be set by a new property, that would be great.

@sandwwraith
Copy link
Member

sandwwraith commented Mar 4, 2024

Can't it always be assumed that when it's nullable, it should default to null? As when you mark something nullable, you are fully aware it can be missing and that nulls could be present.

IMO, this is a very far-reaching assumption. Json has a clear distinction between null and absence; while these may be the same things for one API, they may be different for another (hence the necessity for all these flags). Moreover, we strive to be format-agnostic in the core, and for other data formats this distinction may be even more important.

@pdvrieze
Copy link
Contributor

pdvrieze commented Mar 4, 2024

Technically there is a difference between a value set to null and the value being nullable (without null default), but is this a concern to the library in this case?

Can't it always be assumed that when it's nullable, it should default to null? As when you mark something nullable, you are fully aware it can be missing and that nulls could be present.

A null value is not the same as a default value. And there are cases where the default would not be null (generally used to set the absence of a value). There are similarly cases where assuming absence of a value equals null is incorrect (this is somewhat format specific, explicitNulls provides this).

More to the point, it is not clear that it is safe to assume that null is the same as a default, or that it is safe to treat an unsupported value the same as the absence of said value (without it being signalled, it is not possible for the relying code to know this substitution happened).

@boy12hoody
Copy link

boy12hoody commented Mar 13, 2024

It's just not about enums only.

explicitNulls = false

doesn't do what it says right now

When this flag is disabled [...] during decoding, the absence of a field value is treated as null for nullable properties without a default value.

val overdueDays: Int? gives me
kotlinx.serialization.MissingFieldException: Field 'overdueDays' is required for type with serial name ...

Why is this labelled as a feature when it is clearly a bug?

@sandwwraith
Copy link
Member

@boy12hoody The original issue is specifically about an incorrect enum value in the input and is not related to Ints. If you believe you've encountered another bug, please open a new issue with reproducer.

@carstenhag
Copy link
Author

@sandwwraith I just hadn't noticed other types also having this issue. Should I reword my issue or do you want a new one (from me or boy12hoody)?

@sandwwraith
Copy link
Member

sandwwraith commented Mar 13, 2024

@carstenhag It's better to create a new one. So far, I'm unable to reproduce what you are saying:

@Serializable
    data class NullableInt(val i: Int?)

    @Test
    fun testNullableInt() {
        val j = Json { explicitNulls = false; coerceInputValues = true }
        println(j.decodeFromString<NullableInt>("{}"))
        println(j.decodeFromString<NullableInt>("{\"i\":null}"))
    }

correctly yields NullableInt(i=null) for me.

@boy12hoody
Copy link

@sandwwraith I dont use coerceInputValues but setting it true didn't do anything and the bug applies to any type, not just Int. My setup is:
kotlinxSerializationJson = "1.6.3"
kotlin = "1.9.22"
ktor = "2.3.9"

@sandwwraith
Copy link
Member

@boy12hoody As I said, please create a separate issue with a code to reproduce

@dennisbellmann
Copy link

Regarding to the issue described by @carstenhag
And the answer from @sandwwraith

We in our project highly relied on the combination of
coerceInputValues=true and explicitNulls=false and I found it rather intuitive, that it also includes enums.
That's not the case anymore and it feels that i know have to read the documentation more carefully than ever.
Also a problem is, that this "feature" went away in a bugfix. If we would not have had a Unittest for this case, we would knew it too late.

I would love to have this "feature" (or bugfix) very soon. 🙏 Thank you in advance!

@sandwwraith sandwwraith self-assigned this Apr 24, 2024
sandwwraith added a commit that referenced this issue May 3, 2024
- Bring back its interaction with coerceInputValues flag
- Enhance documentation and add more samples to it
- Remove @ExperimentalSerializationApi

Fixes #2636
Fixes #2586
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants