Open sithmein opened 4 years ago
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.
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.
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);
}
}
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.
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.
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.
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.
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.
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.
Commit c9e71ed6c60745d7565355b1de01329c2d02b623 broke the deserialization of
JsonObject
parameters. Before this change missing properties were passed anull
to constructors. NowJsonValue.NULL
is passed which causesIllegalArgumentExceptions
during reflective instantiation becauseJsonValue.NULL
is not aJsonObject
.