Open mloho opened 1 year ago
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.
@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.
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));
}
@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:
Optional
, Scala Option
, JDK AtomicReference
) should deserialize from absent case into null
or non-nullOptional
) or take in "absent" value? (Optional<NullNode>
).I am guessing all 3 choices would have proponents:
null
would differentiate between incoming value: (missing/absent vs JSON null
)Optional.empty()
is probably most logical for most users; avoids nulls, does not bring in something that doesn't existOptional<NullNode>
has its benefits too, for odd cases like Optional<Optional<X>>
The question, then, is:
DeserializationFeature
s ?)On Configurability we could probably go with 2 features (even if only 3 out of 4 combinations are usable):
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
.
Example:
When deserialized from:
{}
Expected:myField.isPresent() == false
Actual:myField.isPresent() == true
This is because
myField
gets set to anOptional
of aNullNode
After spending some time looking into the source code of both the
jackson-databind
and thejackson-datatype-jdk8
libraries, the problem seems to lie in theOptionalDeserializer
(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#L203The
OptionalDeserializer
is not overriding its inheritedgetAbsentValue
method to returnOptional.ofNullable(_valueDeserializer.getAbsentValue(ctxt));
(or similar).Due to the lack of the overriding, the inherited
getAbsentValue
method actually callsgetNullValue
instead as can be seen here: https://github.com/FasterXML/jackson-databind/blob/0fe97e0d69b7d5362907b094d5b979bc2216dc4a/src/main/java/com/fasterxml/jackson/databind/JsonDeserializer.java#L349In the case of a
JsonNode
, theJsonNodeDeserializer
is used. This deserializer overrides thegetNullValue
method to return aNullNode
.https://github.com/FasterXML/jackson-databind/blob/0fe97e0d69b7d5362907b094d5b979bc2216dc4a/src/main/java/com/fasterxml/jackson/databind/deser/std/JsonNodeDeserializer.java#L73