Optional<JsonNode> deserialization from "absent" value does not work in the expected way

[mloho]

Example:

public record MyRecord(
        Optional<JsonNode> myField
) {
}

When deserialized from: {}
Expected:
myField.isPresent() == false
Actual:
myField.isPresent() == true

This is because myField gets set to an Optional of a NullNode

After spending some time looking into the source code of both the jackson-databind and the jackson-datatype-jdk8 libraries, the problem seems to lie in the OptionalDeserializer (or higher).

During deserialization, when a property is missing, the PropertyValueBuffer::_findMissing method is called and in it, this piece of code is called:
https://github.com/FasterXML/jackson-databind/blob/0fe97e0d69b7d5362907b094d5b979bc2216dc4a/src/main/java/com/fasterxml/jackson/databind/deser/impl/PropertyValueBuffer.java#L203

            // Third: NullValueProvider? (22-Sep-2019, [databind#2458])
            // 08-Aug-2021, tatu: consider [databind#3214]; not null but "absent" value...
            Object absentValue = prop.getNullValueProvider().getAbsentValue(_context);
            if (absentValue != null) {
                return absentValue;
            }

The OptionalDeserializer is not overriding its inherited getAbsentValue method to return Optional.ofNullable(_valueDeserializer.getAbsentValue(ctxt)); (or similar).

Due to the lack of the overriding, the inherited getAbsentValue method actually calls getNullValue instead as can be seen here:
https://github.com/FasterXML/jackson-databind/blob/0fe97e0d69b7d5362907b094d5b979bc2216dc4a/src/main/java/com/fasterxml/jackson/databind/JsonDeserializer.java#L349

    @Override
    public Object getAbsentValue(DeserializationContext ctxt) throws JsonMappingException {
        return getNullValue(ctxt);
    }

In the case of a JsonNode, the JsonNodeDeserializer is used. This deserializer overrides the getNullValue method to return a NullNode.

https://github.com/FasterXML/jackson-databind/blob/0fe97e0d69b7d5362907b094d5b979bc2216dc4a/src/main/java/com/fasterxml/jackson/databind/deser/std/JsonNodeDeserializer.java#L73

    @Override
    public JsonNode getNullValue(DeserializationContext ctxt) {
        return ctxt.getNodeFactory().nullNode();
    }

[cowtowncoder]

Hmmmh. I think you are right in that handling of Optional is incorrect. However, looking at AtomicReference handling, I think that in this case it should become null and not Optional.empty().
I will file an issue for changing this for 2.14; and perhaps also a configuration setting to allow keeping the old behavior since no doubt someone somewhere is counting on that.

[cowtowncoder]

@mloho I implemented #251 for upcoming 2.14. It will allow changing behavior; however, in that case value becomes Java null. This can be combined with @JsonSetter(nulls = Nulls.EMPTY) which I think does do what (I think...) you are looking for.

It is frustratingly difficult to figure out behavior that everyone would find intuitive here...
There probably needs to be some form of further configurability options in future.

[mloho]

It is frustratingly difficult to figure out behavior that everyone would find intuitive here...
There probably needs to be some form of further configurability options in future.

@cowtowncoder, agreed! Configurability typically covers most if not all use cases (depending on the extensiveness of the config / complexity of the domain). However, for default intuitiveness, I always try to refer to the documentation..

I'm not sure if the JLS states best practices for Optionals, but if I try to return a null from a method that returns Optional<Whatever>, IntelliJ IDEA would give me a warning (Null is used for 'Optional' type in return statement), not sure if that's from Jetbrains or the JLS itself, but I've been honoring this warning.

In any case, (in my opinion) having an Optional<JsonNode> attribute be deserialized by default into Optional.of(new NullNode()) if the attribute doesn't exist, defeats the entire purpose of using Optionals.

My OptionalDeserializer patch has been working great so far!

    @Override
    public Object getAbsentValue(DeserializationContext ctxt) throws JsonMappingException {
        return Optional.ofNullable(_valueDeserializer.getAbsentValue(ctxt));
    }

[cowtowncoder]

@mloho Problem is that this is a behavior change that affects all users with existing usage, making it much less desireable to change the default behavior. Even if I agreed your choice is better default on its own, the breaking chance (for some users) may outweigh benefits of change.

But wrt configuration we have 2 aspects to consider:

1. Whether refererential type (Optional, Scala Option, JDK AtomicReference) should deserialize from absent case into null or non-null
2. If non-null, what about content? Should it be simply left out ("empty" Optional) or take in "absent" value? (Optional<NullNode>).

I am guessing all 3 choices would have proponents:

1. null would differentiate between incoming value: (missing/absent vs JSON null)
2. Optional.empty() is probably most logical for most users; avoids nulls, does not bring in something that doesn't exist
3. Optional<NullNode> has its benefits too, for odd cases like Optional<Optional<X>>

The question, then, is:

1. How to configure (multiple DeserializationFeatures ?)
2. What would be the default.

On Configurability we could probably go with 2 features (even if only 3 out of 4 combinations are usable):

• Absent reference type value to null? true/false
• Content of absent reference type to null? (or absent) true/false -- only matters if first setting is "to empty"

and I would think that for backwards-compatibility reason we would probably want both to be enabled by default.

If above makes sense, would you mind filing a request to jackson-databind? This would require support via adding new DeserializationFeature.