Search before asking

  • [x] I searched in the issues and found nothing similar.

Describe the bug

This is a follow-up to the following issues https://github.com/FasterXML/jackson-databind/issues/5139 (fixed via PR #5140) https://github.com/FasterXML/jackson-module-kotlin/issues/976

In the deserializer inheriting from ContainerDeserializerBase, I found problems in the following classes

The StringArrayDeserializer does not inherit from the ContainerDeserializerBase, but seems to have the same problem. There may be other deserializers with similar problems, but I have not been able to verify them.

The MapEntryDeserializer was not using _nullProvider. However, it has not been confirmed that there is no problem with not using _nullProvider.

Version Information

2.19.0(2.19.1-SNAPSHOT)

Reproduction

Same as https://github.com/FasterXML/jackson-databind/issues/5139

Expected behavior

If the deserialization result of the value is null, then JsonSetter.contentNulls must work.

Additional context

No response

Comment From: JooHyukKim

Is there reproduction u can share? If its difficult to convert to Java, even maybe in Kotlin version would be good starting point

Comment From: k163377

For MapDeserializer, it is easy to reproduce since the conversion from empty string to Integer returns null(see this). The ObjectArrayDeserializer may be reproduced in the same way.

As for the others, I don't think they can be reproduced without defining a custom deserializer that returns null for the value.

If you need all the reproducible cases, please give me some time to create them. I have a few things to do today, so all submissions will probably be due at the end of next week.

Comment From: cowtowncoder

I think creating one test, listing other likely types, is fine. Test PR can be against 2.19; can then see where fixes should go (2.19 or 2.x depending on how big/risky changes are).

Comment From: k163377

A PR has been issued to add a failing test.

5174

The EnumSetDeserializer was not reported initially, but a case was added because of a similar problem.

Comment From: k163377

@cowtowncoder I would like to discuss the policy of modifying EnumSetDeserializer. In the following code, _nullProvider is not called, should it be called?

   // First: since `null`s not allowed, slightly simpler...
   if (p.hasToken(JsonToken.VALUE_NULL)) {
       return (EnumSet<?>) ctxt.handleUnexpectedToken(_enumType, p);
   }
   try {
       Enum<?> value = _enumDeserializer.deserialize(p, ctxt);

https://github.com/k163377/jackson-databind/blob/34ae9f52db2c1b55a81d746d01e39e1b6d074a51/src/main/java/com/fasterxml/jackson/databind/deser/std/EnumSetDeserializer.java#L273-L285

Also, it seems that the implementation policy is not unified, not only in this part, but also whether null should be skipped or an error, and what _skipNullValues means within this class. Personally, I feel the following is correct, but what do you think?

flowchart TD
    A[Is JsonToken.VALUE_NULL?] -->|Yes| B[Is _skipNullValues true?]
    B -->|Yes| C[Skip]
    B -->|No| D[value = _nullProvider.getNullValue]

    A -->|No| E[value = _enumDeserializer.deserialize]
    E --> F{Is value null?}
    F -->|Yes| D
    F -->|No| M[Use value]

    D --> I{Is value null?}

    I -->|Yes| J[Is _skipNullValues true?]
    J -->|Yes| K[skip value]
    J -->|No| L[Throw error]

    I -->|No| M

However, if this flowchart were implemented as is, the errors thrown would change. We need to consider whether we want a complex implementation instead of a destructive change, or a simple implementation instead of a destructive change. Also, this issue is a bit different from what I wanted to fix in terms of kotlin-module, so I think it should be a separate issue.

Comment From: cowtowncoder

@k163377 Thank you very much for doing the diagram! I think that is exactly correct, but I also agree with your concerns.

So how about this:

  1. For 2.19, let's not change EnumSetDeserializers behavior wrt null check, to keep patch version behavior consistent.
  2. For 2.20 (2.x) let's change behavior to more correct. I don't think this is major breaking change, but it is change big enough to go into minor update, not patch.

Comment From: k163377

@cowtowncoder The last PR on this Issue has been submitted.

5202

So how about this:

Ok. A new issue has been submitted.

5203