Deserialization of `JsonObject` from `null` broken since 2.10.4

[sithmein]

Commit c9e71ed6c60745d7565355b1de01329c2d02b623 broke the deserialization of JsonObject parameters. Before this change missing properties were passed a null to constructors. Now JsonValue.NULL is passed which causes IllegalArgumentExceptions during reflective instantiation because JsonValue.NULL is not a JsonObject.

[cowtowncoder]

Which version? Can you provide a unit test (or at least show code) to show the exact problem -- description can be helpful but I am not sure how to reproduce with just above.

[sithmein]

Here's a minimal reproducer: jsr353.zip

It only contains a test class that shows the problem. It worked until 2.10.3 and is broken since 2.10.4.

[cowtowncoder]

Thank you @sithmein

(from above link)

package jsr353;

import javax.json.JsonObject;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.datatype.jsr353.JSR353Module;

public class Test {
	public static class Pojo {
		@JsonCreator
		public Pojo(@JsonProperty("s") String s, @JsonProperty("o") JsonObject o) {
			// does nothing
		}
	}

	@org.junit.Test
	public void testDeserialization() throws Exception {
		ObjectMapper mapper = new ObjectMapper();
		mapper.registerModule(new JSR353Module());
		
		// this works
		String s = "{\"s\": \"String\", \"o\": { \"a\": 1, \"b\": \"2\" } }";
		mapper.readerFor(Pojo.class).readValue(s);
		
		// this doesn't since 2.10.4
		s = "{\"s\": \"String\"}";
		mapper.readerFor(Pojo.class).readValue(s);
	}
}

[cowtowncoder]

Ah. This may be tricky thing to fix -- and very unfortunate breakage (wrt fix for #14) in patch release. Thank you for reporting this.

I think I know what the issue is, and maybe how it could be addressed... but at least this test helps.

[sithmein]

There's also another issue with this change: Code that relied on getting null for a missing JsonValue now suddenly gets a non-null object and may therefore behave differently. I see the use of passing JsonValue.NULL but then it should be configurable with the old behaviour being the default. Not sure how you can configure modules, though.

[cowtowncoder]

You can register a custom deserializer for that. While modules can be configured during construction, I don't think I want to give an option here. However, in hindsight, change should not have been made in a patch release, but in minor version.

[sithmein]

Well, the change will break existing code no matter what and it will break during runtime and not compilation (which is worse). If Jackson followed semantic versioning (not sure if it does) then this would deserved a major version bump. In any case it should be mentioned prominently in the release notes.

[cowtowncoder]

Jackson does follow semantic versioning, and change was to fix bug: null for Tree Models should ideally be represented with "null node": this is how JSON nulls have always been represented for property values within JsonObject and ArrayObject -- but before change, not for "root values". Due to the way @JsonCreator works, root value (-like) handling also applies to "missing" values. Intent would have been to keep all these consistent.

I realize that this is gray area -- what I see as a bug you see as feature -- but I want to stress reasoning from my side: it was not (meant as) arbitrary change to semantics. It is just unfortunate that semantics of binding were not established earlier on, for this particular case.

Having said that, If you do feel strongly there should be a switch to allow alternate behavior (expose as nulls), I am open for a PR, but due to semantic versioning that will need to go in 2.12.0 (API additions should not go in patches). As to 2.10: 2.10.5 will revert the change to keep 2.10 behavior consistent minus 2.10.4. 2.11.x will continue with mapping to JsonValue.NULL where possible (it is NOT possible if declared as ObjectValue since assignment can not succeed). Behavior in 2.12.0 could be made configurable, at least, but I do not see full revert to 2.10 mode as being beneficial.

[sithmein]

I found an (easy) way to work around (check for both null and JsonValue.NULL) therefore for me it really doesn't matter right now. I'm just concerned that others may fall into the same trap. Therefore my suggestion was to at least mention the potential issue somewhere in the release notes. Having configurable modules would be nice, though, but that's a different topic.