Regression on deserialization with Include.NON_DEFAULT

[BorisNaguet]

Hello

I have a regression on de-serialization.
When a field has a default value (say int i=1), and it's changed to the default value of the type (i=0), serializing with the global NON_DEFAULT (objectMapper.setSerializationInclusion(Include.NON_DEFAULT)) and then de-serializing doesn't get the same object.

I've tracked down the problem and it appeared with 2.8.3.
I'll follow with a test case that works with 2.8.2, 2.7.x and even an old 2.5.4
The bug is still present with 2.8.7 and master (2.9.0.pr3-SNAPSHOT)

It's certainly related to #1351 (that was released in 2.8.2) and following #1417

[BorisNaguet]

IMHO the regression is worst than what has been solved because Include.NON_DEFAULT is just an optimization to reduce the size of json, but now serializing/de-serializing doesn't get you the same result and it can introduce some hard to understand bugs (at least in my project it was a hard one :) )

[BorisNaguet]

Simplified test case for the discussion:

public class TestSerialization {
    public static class Holder {
        int i1 = 1;
        public int getI1() {
		return i1;
	}
    }

    @Test
    public void test() throws Exception {
        ObjectMapper objectMapper = new ObjectMapper();
        objectMapper.setSerializationInclusion(Include.NON_DEFAULT);

        Holder holder = new Holder();
//default value to default int
        holder.i1 = 0;

        String asString = objectMapper.writeValueAsString(holder);
        System.out.println(asString); // "{}"

        Holder readValue = objectMapper.readValue(asString, Holder.class);
        System.out.println(readValue.i1); // "1"
        assertEquals(holder.i1, readValue.i1);
    }
}

[cowtowncoder]

@BorisNaguet Unfortunately definition of NON_DEFAULT is rather complicated: it can only define defaults of enclosing POJO when applied specifically to that class. When applied as default setting it can and will use default of the value itself -- in case of int that would be 0.

Specific reason for this is technical, and has to do with difficulty of determining intended defaults of a POJO (which requires creation of instance using default constructor) in part of code that combines default settings from various levels.

So to achieve effect you want you will have to apply setting as annotation or "configOverride" (ObjectMapper.configOverride(type).setInclude(...)); it will not work as global default.

I think it is unfortunate that exact behavior of inclusion criteria has changed over versions, but this is the intended behavior for now and going forward.

[BorisNaguet]

But still, there's an issue somewhere, because in the test case:
writeValueAsString() -> readValue()
doesn't result in the same value.

Considering I only use the global config, default value for int is 0, so it's normal that it's not serialized, but it should be set to zero after instantiation, am I wrong?

Otherwise, it should be clearly documented somewhere that using global NON_DEFAULT, POJOs should not be initialized before/in default constructor.

[cowtowncoder]

@BorisNaguet No, I don't see anything wrong with the test, with respect to how I explained things work. Perhaps you are thinking that Jackson modifies all properties, even if no value is included in JSON? This is not done: only setters/fields for data that exists are used -- any number of potential properties for which value is not found are left as is. So, if and when value is excluded from serialization, no change is done to that property on deserializer, and since JVM initializes value that will remain as is.

As to javadocs: I am not opposed to adding warning regarding usage, but there are literally unlimited number of ways one can construct POJOs so as to result in non-identical contents with write/read sequence. So this is not something specific to use of JsonInclude.

[BorisNaguet]

Hello,
I understood your message.

My point was that I don't find it logical.

IMHO, there can be 2 behaviors (in pseudo-java-javascript-code):

A - Don't serialize when value is default of the instantiated object

serialize(Object obj) {
  String result = "{}";
  ref = obj.class.newInstance(); //needs a default construct
  foreach(p : obj.properties) {
    if(obj[p.name] != ref[p.name]) {
      addToJson(result, obj.p);
    }
  }
  return result;
}

deserialize(String str, Class clazz) {
  result = clazz.newInstance() //needs a default construct
  foreach(p : getJsonProps(str)) {
    result[p.name] = p.value;
  }
  return result;
}

B - Don't serialize when value is default the type

serialize(Object obj) {
  String result = "{}";
  foreach(p : obj.properties) {
    if(obj[p.name] != p.type.defaultValue) {
      addToJson(result, obj.p);
    }
  }
  return result;
}

deserialize(String str, Class clazz) {
  result = clazz.newInstance() //needs a default construct
  jsonProps = getJsonProps(str);

  foreach(p : result.properties) {
    if(jsonProps.contains(p.name)) {
      result[p.name] = jsonProps[p.name];
    }
    else {
      //yes in this case we set every property
      result[p.name] = p.type.defaultValue; //override any default initialized in construct/class
    }
  }
}

I don't care if it's A or B (B is more "portable" though if deserialized without the class definition), but the test case shows that serialization uses B (great), but deserialization looks like a A.

I know it's more complicated than that of course :)
And my usage is very simple (one global config and nothing else - no annotation).
So I'll try configOverride, thanks

[cowtowncoder]

@BorisNaguet I am not arguing that the behavior is the most logical possible (I understand why this is not the case); but I am explaining how it works from perspective of what is possible to support from internal handling perspective.

[BorisNaguet]

OK, the proposed solution is working for me:

Value non_default = JsonInclude.Value.construct(Include.NON_DEFAULT, Include.NON_DEFAULT);
objectMapper.configOverride(Holder.class).setInclude(non_default);

Thanks

[cowtowncoder]

@BorisNaguet yes. Apologies for confusion here: original implementation for NON_DEFAULT only focused on use for classes directly, which can be made to work quite well. But extending inclusion criteria for properties and as global default turned out much more complex and having more limitations... so combination is not very intuitive. But I hope it works for your case.