For an absent property Jackson injects NullNode instead of null to a JsonNode-typed constructor argument of a @ConstructorProperties-annotated constructor

[robvarga]

Describe the bug
When using @ConstructorProperties-annotated constructor to deserialize type, for an attribute of type JsonNode which is absent from the deserialized JSON, a NullJsonNode is passed to the constructor instead of null reference.

I would like to know if there is a workaround (e.g. some annotation indicating what to do for absent properties, or configuring some alternative deserializer on a per-property basis which is able to deal with this) for this issue which works with a constructor and final attributes instead of having to resort to non-final attributes and setters.
Since our project is on 2.7.5 at the moment, I would particularly be intereted if there is a workaround for 2.7.x which allows me to inject null instead of this NullNode at least on a per object basis if not on a per-property basis.

If there is no workaround, then this should probably be fixed or a workaround should be provided which works with final attributes and @ConstructorProperties-annotated constructor.

Version information
Which Jackson version(s) was this for?
Reproduced with 2.12.4, 2.9.8 and 2.7.5

To Reproduce
If you have a way to reproduce this with:

package jacksontests;
import java.beans.ConstructorProperties;
import org.junit.Assert;
import org.junit.Test;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;

public class JsonNodeNullTest {

    public static class TestClass {

        private final JsonNode nodeFromConstructor;
        private JsonNode nodeFromSetter;

        @ConstructorProperties("nodeFromConstructor")
        public TestClass(JsonNode nodeFromConstructor) {
            this.nodeFromConstructor = nodeFromConstructor;
        }

        public JsonNode getNodeFromSetter() {
            return nodeFromSetter;
        }

        public void setNodeFromSetter(JsonNode nodeFromSetter) {
            this.nodeFromSetter = nodeFromSetter;
        }

        public JsonNode getNodeFromConstructor() {
            return nodeFromConstructor;
        }
    }

    @Test
    public void testJsonNodeNulls() throws Exception {
        TestClass res = new ObjectMapper().readValue("{}", TestClass.class);
        Assert.assertNotNull(res);
        Assert.assertNull(res.getNodeFromSetter());
        Assert.assertNull(res.getNodeFromConstructor());
    }
}

Expected behavior
The unit test fails as res.getNodeFromConstructor() returns a NullNode instead of null.
It should return null identically to the property which is setter-populated. (The setter does not get called).

Additional context

[cowtowncoder]

I cannot think of a way to change this behavior, except for custom JsonNode deserializer (which would work but requires some effort).
Given that this behavior has existed for a long time (based on versions mentioned), I doubt change in behavior is possible for patches, although could perhaps be considered for a minor version.

But I hope to have a look at this before 2.13.0 is released to double-check my understanding. Null replacement itself is needed mostly for primitive values; I think I agree that it should not be used for JsonNode.

[robvarga]

Ok, the following JsonDeserializer referenced with @JsonDeserialize#using seems to do the trick:

public class AbsentAsNullJsonNodeDeserializer extends JsonNodeDeserializer {

    /**
     * This I believe is required with Jackson 2.9 upwards in order for #getNullValue(DeserializationContext) to be invoked on every property and with a non-null <code>ctxt</code> parameter.
     * @since 2.9 
     * /
    @Override
    public AccessPattern getNullAccessPattern() {
        return AccessPattern.DYNAMIC;
    }

    @Override
    public JsonNode getNullValue(DeserializationContext ctxt) {
        return ctxt.getParser().hasToken(JsonToken.END_OBJECT) ? null : super.getNullValue(ctxt);
    }
}

Thanks for the clue that it should be possible with JsonDeserializer, I looked at it once but didn't succeed to get it done at that time.

Tested with Jackson 2.7.5 and 2.12.4

[robvarga]

A unit test to cover the main possible cases:

public class JsonNodeNullTest {

    public static class TestClass {

        private final JsonNode nodeFromConstructor;
        private final JsonNode nodeFromConstructor2;
        private JsonNode nodeFromSetter;
        private JsonNode nodeFromSetter2;

        @ConstructorProperties({ "nodeFromConstructor", "nodeFromConstructor2" })
        public TestClass(@JsonDeserialize(using = AbsentAsNullJsonNodeDeserializer.class) JsonNode nodeFromConstructor,
                         @JsonDeserialize(using = AbsentAsNullJsonNodeDeserializer.class) JsonNode nodeFromConstructor2) {
            this.nodeFromConstructor = nodeFromConstructor;
            this.nodeFromConstructor2 = nodeFromConstructor2;
        }

        public JsonNode getNodeFromSetter() {
            return nodeFromSetter;
        }

        public void setNodeFromSetter(JsonNode nodeFromSetter) {
            this.nodeFromSetter = nodeFromSetter;
        }

        public JsonNode getNodeFromSetter2() {
            return nodeFromSetter2;
        }

        public void setNodeFromSetter2(JsonNode nodeFromSetter2) {
            this.nodeFromSetter2 = nodeFromSetter2;
        }

        public JsonNode getNodeFromConstructor() {
            return nodeFromConstructor;
        }

        public JsonNode getNodeFromConstructor2() {
            return nodeFromConstructor2;
        }
    }

    @Test
    public void testJsonNodeNulls() throws Exception {
        TestClass res = new ObjectMapper().readValue("{\"nodeFromConstructor2\" : null, \"nodeFromSetter2\" : null }", TestClass.class);
        Assert.assertNotNull(res);
        Assert.assertNull(res.getNodeFromSetter());
        Assert.assertThat(res.getNodeFromSetter2(), CoreMatchers.instanceOf(NullNode.class));
        Assert.assertNull(res.getNodeFromConstructor());
        Assert.assertThat(res.getNodeFromConstructor2(), CoreMatchers.instanceOf(NullNode.class));
    }
}

[cowtowncoder]

I will have to think about this a bit more -- may consider change in 2.14, but too late for anything in 2.13.

[robvarga]

Just don't break the workaround, please :-)

[cowtowncoder]

@robvarga :)

Usage as shown should still work.

Another work-around, if you don't mind using Optional, would be using Optional<JsonNode> -- I think that would pass absent Optional as expected.

[novtor]

We have the same issue starting from 2.11. Koltin class containing nullable property is desirialized as NullNode instead of null. For example:
data class AModel(val aProp: com.fasterxml.jackson.databind.JsonNode?)
when desirializing from {} string results in aProp to be NullNode.instance

[cowtowncoder]

One problem here wrt Kotlin is just that Kotlin has additional nullability metadata over Java.

So the question of null mapping into JsonNode is one thing, but whether to, and how, consider Kotlin metadata is similarly tricky problem.

[novtor]

sure, @cowtowncoder, finally I open a PR in jackson-databind-kotlin: FasterXML/jackson-module-kotlin#491.
I think that if a value is nullable and missing in the payload, it should not be assigned any null-like value

[cowtowncoder]

I'll let Kotlin module maintainers to decide that: I think it is something worth documenting. The original use case for getNullValue() really was about primitive values (for which null does not work) and creator methods.
But it then extended a bit on "reference types" (Optional of Java 8, Guava, Option of Scala), and semantics there get bit muddier, since null-avoidance is sort of what these types do. Whether same should apply to JsonNode is debatable.

[novtor]

Agree, so let us debate 😄
If we look at it from the e2e serialize/deserialize point of view: null value is serialized as null (or absent) and should be deserialized as null. Currently, missing or null payload is deserialized as NullNode.
Another point is that it is like this only for kotlin classes, not java, java field is deserialized as null
I think there is some inconsistency

[robvarga]

Agree, so let us debate
If we look at it from the e2e serialize/deserialize point of view: null value is serialized as null (or absent) and should be deserialized as null. Currently, missing or null payload is deserialized as NullNode.
Another point is that it is like this only for kotlin classes, not java, java field is deserialized as null
I think there is some inconsistency

It is worse. Even in Java, it is only deserialized as null (not set) if the property was not part of those injected via the chosen constructor/creator. If the property is among those passed to the chosen constructor, it is deserialized as NullNode, that is what my bug complained about.

I agree, that it should be null, as that represents absent property better when explicit null is represented by NullNode.
I also believe, behavior should be consistent, so it is worth considering invoking the setter with a null for absent properties.

An alternative which could be considered to avoid NPE's is representing absent property as MissingNode.

[cowtowncoder]

I don't disagree with above; and yes, it only affects cases where property value is piped through Creator method.

My only concerns are that I want to

1. Understand why I added getNullValue() for JsonNodeDeserializer originally (if there was another issue addressed) and
2. Know what use case(s) would fail if behavior was changed for 2.13 (do any existing tests start failing)

Right now the practical problem is that I have very little time to work on OSS (this is my hobby, not daytime job) which slows down progress. But I hope to get this resolved before 2.13.0 final. I appreciate all the help so far, let's keep discussion going.

One possibility I am thinking of is to add DeserializationFeature to toggle this behavior (to allow either one); it may default to the new, null-returning behavior. This would make sense if I feel there is a significant risk of regression (some legit usage breaking with the change).
I am not yet sure if that is necessary.

[cowtowncoder]

Hmmmh. There is actually one immediate failure (if changing JsonNodeDeserializer.getNullValue()), and it points out one use case: that of JDK serializability of JsonNode.
In that case NullNode gets serialized as json content of null -- and reading back, that really should become NullNode.
I'll have to think of how to address that use case: it is minor, granted, but I'd want it to work as it should.

If worse comes to worst it might even be necessary to add a hack into handling of Creator arguments to avoid "un-nulling" absent value for specific case of TreeNode-valued creator arguments.

@novtor As to round-trippability of null -- I am not convinced by that line of reasoning as JsonNode representation of null is definitely NullNode, not Java null. For other types situation is different.

[robvarga]

Hmmmh. There is actually one immediate failure (if changing JsonNodeDeserializer.getNullValue()), and it points out one use case: that of JDK serializability of JsonNode.
In that case NullNode gets serialized as json content of null -- and reading back, that really should become NullNode.
I'll have to think of how to address that use case: it is minor, granted, but I'd want it to work as it should.

If worse comes to worst it might even be necessary to add a hack into handling of Creator arguments to avoid "un-nulling" absent value for specific case of TreeNode-valued creator arguments.

Which would be the correct thing to do. Since the constructor invocation do not have the ability to not take absent properties therefore you have to differentiate between absent and explicit null properties with different values passed to that parameter. And since if you declared the attribute ObjectNode or ArrayNode instead of JsonNode then you cannot receive NullNode in the first place anyway, so passing null instead of NullNode to properties declared as JsonNode actually makes behaviour of JsonNode identical to behaviour of ObjectNode/ArrayNode.

@novtor As to round-trippability of null -- I am not convinced by that line of reasoning as JsonNode representation of null is definitely NullNode, not Java null. For other types situation is different.

I am not sure exactly what you mean with this, but IMHO, if the question of how to interpret a serialized null comes up at all in a round-trip scenario, that means that your serialization is the cause of the problem as that loses the information in the first place whether the object which was serialized had a null or a NullNode in the property by serializing both possibilities the same way.

[cowtowncoder]

I am now thinking of adding JsonDeserializer.getAbsentValue(ctxt) (or maybe getMissingValue(ctxt)), which would then allow differentiating the use cases.

[novtor]

My concern is that JsonNode and 'JsonNode?' are not the same thing. Personnaly, I would even say that Optional and 'Optional?' should behave differently even if it is a nonsense to declare a field as 'Optional?'

Let us imagine the kotlin cases. I am omitting the fields with default value as it should be strightforward, if a field is missing, it is given the default value, otherwise it should behave as for special or regular values:

Field declaration	Missing field	Special values	Feature to change default behavior
JsonNode	exception	null -> NullNode
JsonNode?	null	null -> null
Optional	Optional.empty()	null -> exception
Optional?	null	null -> null

For Java cases, as there is no non-nullable types, everything should behave as nullables in kotlin.

And finally, @robvarga 's point about NullNode serialization, I join her/him. Probably the issue is there: should it be serialized as an empty string or {} but not as null ?

[novtor]

I think we can join here this one also #2705

[cowtowncoder]

On NullNode... to me it absolutely should be serialized as null, JsonNode is supposed to be exact representation of the content. MissingNode represents something missing/absent.

[robvarga]

I am now thinking of adding JsonDeserializer.getAbsentValue(ctxt) (or maybe getMissingValue(ctxt)), which would then allow differentiating the use cases.

I guess this is one way of handling the backward compatibility aspect, but ideally it should be supported by the corresponding out-of-the-box enums used in annotations to declaratively indicate the possible behaviours without having to write our own JsonDeserializer.

[cowtowncoder]

Agreed that eventually it'd be nice to have more granular mechanisms, and that requiring to provide one's own custom deserializers is not much of an extension point. So the focus here is solely to solve the immediate problem. Later on if there is need for further customizations it'd be possible to discuss value and effort needed for other extension points.
Also note that if annotation support (etc) was added, it would override defaults that deserializers provide, so this does not preclude later improvements.

[robvarga]

On NullNode... to me it absolutely should be serialized as null, JsonNode is supposed to be exact representation of the content. MissingNode represents something missing/absent.

I agree that a present NullNode property value should be serialized to an included null in the JSON.
On the other hand, I believe an absent property could never have originated from a present NullNode property value, so it is wrong to pass NullNode to the creator/constructor parameter for an absent property.
And we already have a perfectly accepted way of letting the user decide what to do when serializing a Java null property value, with the serializationInclusion serialization feature. If he decides to lose the distinction between null an NullNode by forcing serialization of Java null in that property, then we can assume that he knows what he does.

Let's look at it from the other end, the Java class, too.
I don't want to have to have Optional<JsonNode> to be able to deal with something not there in a JSON. I particularly do not want to complicate my code and pollute my heap with the Optional wrapper object, when NullNode.getInstance(), MissingNode.getInstance() and null are all possible right-values to a JsonNode left-value and we have only two cases to represent with those 3 right-value candidates.
It is a perfectly reasonable expectation, that if something is not there in the JSON, then a selectable Java null or MissingNode.getInstance() be passed to the corresponding constructor/creator parameter, or possibly even to a corresponding setter instead of NullNode.getInstance().

This may tie in somewhat to #2430 which I noticed when browsing around, which I believe was the cause of absent properties being converted to NullNode in the first place, but that in my opinion is clearly wrong to apply in the situation of absent properties.
Also #2458 is related and from there transitively #2024 could also be reviewed.

But in my opinion, it is wrong in default behaviour to coerce absent properties to some other values (but #2458 does just that) since they are not null values to start with. Particularly, since they are not coerced when they are populated via setters. It is particularly wrong to coerce them, if serialization inclusion for the created type is set Include.NON_NULL only since it implies that whatever is absent was null originally.

[cowtowncoder]

@robvarga I think you are arguing with something else than what I am saying. :)

I agree that handling of absent values is distinct and separate from handling of incoming null values.
I am only trying to explain the background here which might explain how existing issue came about. And not about absent/null were identical cases: I fully agree they are not (although acknowledging that sometimes being able to tell these apart, end-to-end, is challenging).

I hope to have enough time to work on this now and possibly solve the specific case of NullNode-from-absent case.
I think it does also tie into Optional types as well.

[cowtowncoder]

Ok, proposed fix checked in 2.13 branch -- if anyone has time to check, that would much appreciated.

One tricky part, which seems to work but is... bit fragile... is that of Null replacement (Nulls.FAIL, Nulls.AS_EMPTY) and so on -- that will still treat absent values as equivalent to nulls. This because users rely on that behavior for null avoidance. My main concern there is just the precedence of handling; all tests pass but I suspect there may be usage cases where combinations might not work as expected.
There is room for using additional logic (this is mostly in PropertyValueBuffer class) so there may be room for improvement. But I cannot do much proactively without finding some edge cases.

Still, I think this is solid improvement for now.

[cowtowncoder]

Oh, also: it may well make sense to change behavior for Optional (wrt ReferenceTypeDeserializer) but would be nice to have a separate issue with someone having an actual issue. Semantics of Optional (etc) are a bit different from JsonNode so it's not 1-to-1.