-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deserialization Not Working Right with Generic Types and Builders #921
Comments
Getting a |
I do face the same problem what is the status of this issue ?? is there any work around?? |
@jaroslawZawila I don't think anyone has been working on this issue. |
Thanks for your response. Could you point me in the direction where do you think is a problem with that?? I may look at this as I really need it to work :D |
@jaroslawZawila I would first verify that this still occurs with 2.7 (that is, 2.7.0-SNAPSHOT from So yeah, it can be bit involved. I hope to get this working for 2.7 regardless, but there is a significant backlog of issues to work on unfortunately. So I appreciate any help you can give! |
I have tried using 2.7.0-SNAPSHOT unfortunately the issue still exists. I will investigate a bit more. |
I can reproduce this with 2.7.0-rc1; added the failing unit test. |
…ic test fails due to a bug in Jackson - FasterXML/jackson-databind#921 - and is currently disabled.
…ic test fails due to a bug in Jackson - FasterXML/jackson-databind#921 - and is currently disabled. (#16)
I did some checking and the sample test case provided by @gilbode and committed in 53fb51f by @cowtowncoder fails for the heads of all the 2.x branches (2.1 through 2.7 -- inclusive). It also fails on master. So this does not appear to be a regression -- not sure if that was clear, at least based on a-k-g's issue #1210. I've been debugging this in the 2.7 branch and in BeanDeserializerFactory's createBuilderBasedDeserializer method the valueType contains the bindings as expected, they are not in beanDesc but looking at the BasicBeanDescription that does not seem unexpected. However, the builderType created in this method does not discover/inherit the bindings from the valueType. As a crude first attempt I tried to build the builder type with the value type's bindings: JavaType builderType = ctxt.getTypeFactory().constructType(builderClass, valueType.getBindings()) However, builderClass is not a ParameterizedType so TypeFactory refuses to perform this construction. Temporarily working around this by exposing _fromClass on TypeFactory publicly yields:
And this fixes the problem! -- and all existing tests continue to pass -- So aside from coming up with a cleaner way in TypeFactory to allow this combined type creation (builder base class and value type bindings) there is the question of what assumptions this introduces between the type bindings of the value type and builder. Consider:
@JsonDeserialize(builder = MyGenericPOJO_B.Builder.class)
public static class MyGenericPOJO_B<T> {
private List<T> data;
private MyGenericPOJO_B(List<T> d) {
data = d;
}
public List<T> getData() {
return data;
}
public static class Builder<U> {
private List<U> data;
public Builder<U> withData(List<U> d) {
data = d;
return this;
}
public MyGenericPOJO_B<U> build() {
return new MyGenericPOJO_B<U>(data);
}
}
}
public void testWithBuilder_B() throws Exception {
final ObjectMapper mapper = new ObjectMapper();
final String json = "{ \"data\": [ { \"x\": \"x\", \"y\": \"y\" } ] }";
final MyGenericPOJO_B<MyPOJO> deserialized =
mapper.readValue(json, new TypeReference<MyGenericPOJO_B<MyPOJO>>() {});
assertEquals(1, deserialized.data.size());
Object ob = deserialized.data.get(0);
assertNotNull(ob);
assertEquals(MyPOJO.class, ob.getClass());
}
public static class MyGenericPOJOWithCreator<T, U> {
private List<T> data;
private Map<String, U> map;
private MyGenericPOJOWithCreator(List<T> d, Map<String, U> m) {
data = d;
map = m;
}
@JsonCreator
public static <T, U> MyGenericPOJOWithCreator<T, U> create(
@JsonProperty("data") List<T> data,
@JsonProperty("map") Map<String, U> map) {
return new MyGenericPOJOWithCreator.Builder<T, U>().withData(data).withMap(map).build();
}
public List<T> getData() {
return data;
}
public Map<String, U> getMap() {
return map;
}
public static class Builder<U, T> {
private List<U> data;
private Map<String, T> map;
public Builder<U, T> withData(List<U> d) {
data = d;
return this;
}
public Builder<U, T> withMap(Map<String, T> m) {
map = m;
return this;
}
public MyGenericPOJOWithCreator<U, T> build() {
return new MyGenericPOJOWithCreator<U, T>(data, map);
}
}
}
public void testWithBuilder() throws Exception {
final ObjectMapper mapper = new ObjectMapper();
final String json = "{ \"data\": [ { \"x\": \"x\", \"y\": \"y\" } ], \"map\": { \"a\": 1, \"b\": 2 } }";
final MyGenericPOJO<MyPOJO, Integer> deserialized =
mapper.readValue(json, new TypeReference<MyGenericPOJO<MyPOJO, Integer>>() {});
assertEquals(1, deserialized.data.size());
Object ob = deserialized.data.get(0);
assertNotNull(ob);
assertEquals(MyPOJO.class, ob.getClass());
} The exception shows it tries to deserialize the integer value for the data object:
The type reference for the value type does not imply anything about the builder's generic type bindings. One option that we could consider a BuilderTypeReference to pass such a binding into the deserialization. Another option would be to add annotations to the builder and/or value expressing how the bindings map from the value class to the builder class. However, such measures should only be necessary when the bindings don't map 1:1 in name and order across the value type and builder type. The common case for us at least is that the parameter mapping exists so simply supporting transfer of bindings does not seem like an unreasonable start. If someone can give some guidance on my strategy and how TypeFactory should be extended to supporting merging a value type's bindings with the builder class into a builder type I can submit a PR for this. Otherwise, if I'm barking up the wrong tree here any pointers would be welcome! |
I've searched in vain for a +1 feature on GitHub issues but to no avail. Here goes: I just ran into this issue. Can't tell whether it's a bug or a WAD waiting for improvement. I strongly support fixing this. |
Ok. After re-reading everything above I think I finally understand the problem accurately. Many thanks to @vjkoskela for digging through it. That's good news. As to bad news... to be completely honest I don't know if this will ever be supported. @vjkoskela is correct in pointing out that the issue is that builder type is specified as type-erased class and there is no way to really either indicate or infer type parameterization. It is not safe, in general, to assume that there is strict equivalency of bindings between value and builder type declarations: even if names are same (like In theory one could perhaps figure out type by first fully resolving value type, with generics; then locate build method from builder, and try to backtrack actual type parameterization by matching expected value type into type parameters exposed by build method. This is probably solvable but would require someone to really really dig deep into type handling (reification?). I don't think I'll have time or interest to pursue that myself. Having said that, maybe naive matching of type bindings between value and builder could be acceptable, but only if:
and if above do not match, a hard exception should be thrown to indicate invalid definitions.
|
A possible workaround: Add a factory method with @JsonCreator and then add the builder anyway. |
Why not make the dumb assumption, check the build method returns the correct type given that assumption, and error out if anything looks wrong? |
@ChrisAlice At high level, yes, that would seem sensible, but the problem comes from possible complexity of getting there: if we were talking about non-generic types, yes, easy enough to check. Still, such a setup could be doable and sufficient, worth trying out if anyone has time to work on this. |
Ran into this again and this time the generic builders are nested so my previous work around of just deserializing into the builder and building by hand does not work. The PR #1796 addresses only the common case of when the builder type bindings can be exactly inferred from the value type's bindings. This covers all of our use cases, but does not present a complete solution; for this reason, the functionality is controlled in the PR by a Happy to make any changes you require, but I would love a general thumbs up/down as soon as possible so I know whether we should rely on this behavior (I can run a fork for months if need be while we work through the MR and release). @spharris can you elaborate on your work around? |
I see now that it was just mentioned in the original bug: create a builder that Jackson doesn't know about and also add a constructor annotated with public static class MyGenericPOJOWithCreator<T> {
private List<T> data;
private MyGenericPOJOWithCreator(List<T> data) {
this.data = data;
}
@JsonCreator
public static <T> MyGenericPOJOWithCreator<T> create(@JsonProperty("data") List<T> data) {
return new MyGenericPOJOWithCreator.Builder<T>().withData(data).build();
}
public List<T> getData() {
return data;
}
public static class Builder<T> {
private List<T> data;
public Builder<T> withData(List<T> data) {
this.data = data;
return this;
}
public MyGenericPOJOWithCreator<T> build() {
return new MyGenericPOJOWithCreator<T>(data);
}
}
} |
Thanks @spharris ! I took the example in the tests and expanded it a little to see how it behaved (modified code at the end). I have not nailed down all the semantics of the generic type mapping but here are some preliminary results.
Works:
Does not Work:
The failure seems to occur regardless of whether the pojo itself was parameterized (e.g. MyPOJO) or if MyPOJO were not parameterized at all. The conclusion I am drawing so far is that I have not found where in the data bind code this path infers the generic parameters from the type reference. Any help either here or with understanding my conclusion would be appreciated (@cowtowncoder ?) . If my conclusion is correct, then in my opinion it follows that the MR #1796 probably should be the default behavior -- but I'm open either way -- as it seems to bring builder based deserialization behavior more inline with creator based deserialization. Full modified test (
^^ Executed against tag 2.9 |
@vjkoskela Lots to digest, so I'll try to answer just one question:
Type here has to be resolved via full inspection of the enclosing bean/builder type: methods like I don't know how much this helps, but one thing to verify is that resolved type is correct (although type resolution code is in much better shape now, since 2.7, it's pretty complicated thing and bugs are still possible). But the other part then is whether full type information is (and may be) passed across builder, actual type. This is less clear to me. |
Joining the "me too" for this issue, my implementation is as follows:
Is there a known solution to this, or should I pass the desired class as a method argument? |
@hubbazoot Is that using Builder? Without type definitions that isn't of much help unfortunately. |
@cowtowncoder I am not using Builder. I have a POJO that I am expecting as a result and am expecting back. I greatly simplified the code involved to illustrate the core issue. This was the closest ticket I could find that seemed relevant, and this problem is reproducible on my system. If this has been solved elsewhere or could be resolved by overriding a dependency, that would be fantastic.
I have found a workaround that appears to work, but does slightly undermine the purpose of the template
This is in jackson-databind 2.8.10 from spring-boot-starter-web 1.5.9.RELEASE. |
@hubbazoot Note the title: "Deserialization Not Working Right with Generic Types and Builders" -- this issue is about Builders, so your issue is not same, and likely not related. If you can reproduce the issue with straight Jackson |
I ran into this issue with Spring's |
[removed a comment after realizing I may have misread the test] |
It's been a while since I've had a chance to write some Java. Well, I got back into it, and wasn't long before I was using Jackson. That of course reminded that I may owe you @cowtowncoder a few things. It looks like this was 2.9 and then 2.11. Is there anything on this issue I can do to move it forward and get it included in an upcoming release? |
Looking at #1796, I realize I did merge the patch in |
…t static methods, type variables
Quick note: after unrelated change (fix for #2821), one test here fails ( |
is this fixed in the latest version? |
As per |
When trying to deserialize a generic type using a builder it is deserializing the generic type as a LinkedHashMap instead of the proper type, exact same code deserialized using
@JsonCreator
works fine. There seems to be something missing in the builder code.Test case below demonstrates the problem:
The text was updated successfully, but these errors were encountered: