Open pakorcz opened 2 years ago
I am not sure whether this is a valid idea, will have to defer to @drekbour.
Acknowledged, yet to dig and and respond.
Hi @pakorcz. I refer you to https://github.com/FasterXML/jackson-databind/issues/2976#issuecomment-778614008 and the comments which follow it. The given example (great to see something not involving animals!) makes assumptions on a vanilla usecase that mean it's ok to infer the supertype. Those assumptions will not always hold true and the logic will be chaotic in other configurations.
@cowtowncoder Much as it pains me to say, I don't think the change is stable to include.
I really opened a can o' worms with this didn't I! We should keep collecting these requirements (I think they are all the same requirement really but expressed in usefully different ways as users try to hack around the limitations) and perhaps they will weave a clearer picture of where a general-case solution can safely add value.
I take from this issue the reasonable expectation that supertypes might be ok when they are explicitly included in the set of JsonSubType
s.
PS. There exists an alternative implementation of the deduction code which I wrote after the initial PR. It should be faster and would better handle terminal conditions - though nothing can get away from the fact that deser code doesn't why a field is absent given the many options. I'll PR it so it's not lost.
Yeah I was guessing it would not be something we'd want to do for general usage -- there tends to lots of logic that needs to be added to cover all kinds of cases that "make sense" to one user or use case.
+1 for collecting requirements if anyone wants to volunteer -- one possible place would
https://github.com/FasterXML/jackson-future-ideas/wiki
either new JSTEP-x for "improved deduction", or just another wiki page linked. Or an issue.
Ideally I think there should be alternative way to add Type Id detection, given JsonNode
(which does require full buffering but it is what it is), and let users implement more advanced logic.
But that's not easy given existing API design.
I'll raise a new feature because I think I have a workable solution in mind now. It justs needs some meditation before I raise
Creating an issue as Work-in-Progress sounds reasonable; can always edit/change/close/re-create if need be.
Thank you guys for explanation and taking an interest in this case!
The point of it is exactly what @drekbour wrote:
“the expectation that supertypes might be ok when they are explicitly included in the set of JsonSubTypes
”.
I would really appreciate this feature, because thanks to it – if set of possible json data reflects some kind of hierarchy – then it could be deserialized out of the box to proper class of pojo-hierarchy ( without further mapping and checking of null values).
:wave: I wonder if it would be an option to allow opting in to this behavior by explicitly declaring these disambiguating expectations. (Let me first say that while I've been using Jackson since at least as long as I can remember, I've never ventured out here before so apologies if this idea has already been discussed or is fundamentally broken in some way!)
In my understanding the presence of a property is in a natural way a fulfillment of the required
ness of a property, so what if we could resolve cases like the example here by putting the onus on the user, letting them declare CompositePlace
as
class CompositePlace extends Place implements WorthSeeing {
@JsonProperty(required = true)
public Map<String, WorthSeeing> places;
}
// or
public CompositePlace(@JsonProperty(value="places", required=true) Map<String, WorthSeeing> places) {...}
// or
record CompositePlace(@JsonProperty(required=true) Map<String, WorthSeeing> places) implements WorthSeeing {}
which given {"name": "BarkingDuck"}
would then discard CompositePlace
as a candidate since places
is missing. I'd argue that doing it this way would to some extent address concerns re. Include.NON_NULL
, by requiring the user to decide whether an X without a Y is allowed to be an X or not. (Bird/Wingspan
, CompositePlace/Place
, OncallEngineer/PagerNumber
...)
I believe that this could be implemented in a quite similar way to what #3290 does:
Map<BitSet, BitSet>
of required properties per type keyed by fingerprintBitSet actual
candidates
removing if required.andNot(actual)
is not emptycandidates.size() == 1
, otherwise proceedThanks for your great work on this amazing library! :heart:
Thankyou for thinking through the scenario and code. Honouring required=true
is a good addition to the deduction process and goes onto my list.
+1 would love to see this in the next version, can't use the deduction now because the supertypes don't get deducted
+1
I am having the same problem, Is there a solution for it? It seems that when trying to deserialize to a POJO class, it is considering ALL fields (declared and inherited) of all classes, instead of just declared fields. Say type T has declared fields A and B, and subtype S has declared field C. If both declared and inherited fields are considered, just seeing A and B (without C) in the JSON, it cannot infer that T should be used. It thinks that both T and S are candidates, which is wrong. If only declared fields are considered, just seeing A and B, it can deduce that an instance of T must be used. The algorithm for DEDUCT should be something like the one shown below: Let T be the set of types mentioned in JsonTypeInfo (the type itself and its subtypes that we want to deserialize to) For a type t, let declaredFields(t) be the set of all fields declared in type t, and allFileds(t) be the set of declared and inherited fields. Let J be the set of fields the Json document.
@rafiulahad Is it possible to share a reproducible test case with your configuration(includin JsonTypeInfo and Mapper) against Jackson 2.15.2 version? It will be easier to provide help 👍🏻👍🏻😄
@JooHyukKim, below is a minimal reproducible case using Jackson 2.15.2 @Data @JsonTypeInfo(use = JsonTypeInfo.Id.DEDUCTION) @JsonSubTypes({ @JsonSubTypes.Type(SuperClass.class),@JsonSubTypes.Type(SubClass.class)}) public class SuperClass { int a; } @Data public class SubClass extends SuperClass { int b; } public class TestMapper { public static SuperClass deserialize(String json) { ObjectMapper objectMapper = new ObjectMapper(); try { return objectMapper.readValue(json, SuperClass.class); } catch (JsonProcessingException e) { throw new RuntimeException(e); } } }
test code
String json = "{\"a\":1}";
TestMapper.deserialize(json);
result
Could not resolve subtype of [simple type, xxxxxxxx.SuperClass]: Cannot deduce unique subtype of xxxxxxxx.SuperClass
(2 candidates match)
It should use SuperClass in this case.
BTW, it works for json = "{\"a\":1, \"b\":2}";
My algorithm will work for both cases.
(I am not sure why this editor is converting Data to DaTa and backslash double quotes to double quotes :)
Hi Joo Hyuk, I included a simple test case in my comment. Thanks. -Rafiul
On Sat, Jun 17, 2023 at 8:04 AM Kim, Joo Hyuk @.***> wrote:
@rafiulahad https://github.com/rafiulahad Is it possible to share a reproducible test case with your configuration(includin JsonTypeInfo and Mapper) against Jackson 2.15.2 version? It will be easier to provide help 👍🏻👍🏻😄
— Reply to this email directly, view it on GitHub https://github.com/FasterXML/jackson-databind/issues/3289#issuecomment-1595781131, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFPXVTSGEZ2LWAAQRVPPVTTXLXBQDANCNFSM5FASPSXA . You are receiving this because you were mentioned.Message ID: @.***>
@rafiulahad Please read the whole chain for the current status. There has been no work on this, and I do not that without you or someone working on it anything will happen.
Personally I am not sure there should even be lots of more advanced functionality here -- perhaps a better approach would be to allow registering a handler that would be given JsonNode
of all contents, to infer the subtype.
I think it is impossible/impractical to cover all possible cases users would want to use inference on.
But that would be a new feature for someone to implement. I do not have time or actual itch to work on this but maybe someone else does.
I worked around the issue by introducing a field in the super type and not using deduction. Thank you for your insightful feedback as an (the) inventor of this library. To see if I got it right, below are additional details I want to add to your proposal on the handler approach. Suppose we have an interface: Class resolveClass(JsonNode json, Set\<Class> classes) throws .... which when called, will find one class, if it exists, among the classes to use for deserialization. ObjectMapper will call this to get the class (and for fields of type POJO class) to deserialize the given JsonNode. Is this inline with what you are thinking? if so, I can provide a default implementation of the interface. In fact, I have a PoC implementation of it with no effort put in to optimize it yet. But I won't have time to understand the ObjectMapper code to make changes.
@rafiulahad Something like that, yes, although whether Set<Class>
is passed is an open question (resolver need to limit itself to that set, at any rate).
But the tricky part is how to make it all work with @JsonTypeInfo
and existing routing, which assumes different things (like existence of type id, inclusion mechanism); piping through TypeDeserializer
(and possibly TypeSerializer
).
Not so much any implementation of the interace.
The set of classes are the ones mentioned in \@JsonSubTypes
@rafiulahad Yes I was guessing that'd be the case. Note tho that these are typically passed during construction of TypeDeserializer
and not for each resolution call -- this set does not really change so resolver does not need it dynamically passed. But that's a minor detail in grand scheme of things.
Currently, deduction deserialization requires all values to have unique field. Because of that, there is no way to deserialize values to superclass and subclasses, when
defaultImpl
is different than superclass.Example:
For json:
and pojo-hierarchy:
and deduction deserialization with
defaultImpl
differ then supertype:the
MismatchedInputException
is thrown, because the values with onlyname
fields are not classified toPlace
.Desired behaviour:
UnrecognizedPropertyException
is thrown