Skip to content
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

Add DeserializationContext.readTreeAsValue() methods for more convenient conversions for deserializers to use #3002

Closed
cowtowncoder opened this issue Dec 23, 2020 · 7 comments
Milestone

Comments

@cowtowncoder
Copy link
Member

(note: inspired by #3000)

A very common use pattern for deserializers is to read content as a tree (JsonNode) and then extract properties, either simple scalar values (Strings, numbers, boolean), or to further bind as POJOs via delegation.
But while there is a way to achieve that, the "proper" way is somewhat complicated, by:

  1. Constructing JsonParser out of JsonNode (method traverse())
  2. Either locating deserializer to use (find[Non]ContextualValueDeserializer()) and calling it; or using short-cut of readValue() (or readPropertyValue()`)

Because of this, many users instead use construct like:

JsonNode tree = ctxt.readTree(p);
MyPojo value = ctxt.getCodec(). treeToValue(tree.get("pojo"), MyPojo.class);

which has been available since Jackson 2.0, but has some issue regarding not retaining current deserialization context and things like Attributes set.

So it would make sense to add a convenience method that does what is intended starting with existing JsonNode (first checking for null and "missing node"?), locating deserializer & reading.
Naming TBD, could be:

DeserializationContext.treeToValue(JsonNode, Class<?> targetType); // and other method with `JavaType`
@cowtowncoder cowtowncoder changed the title Add a convenient way to convert JsonNode to class (like "valueToTree") via DeserializationContext Add a convenient way to convert JsonNode to class (like treeToValue()) via DeserializationContext Feb 2, 2021
@cowtowncoder
Copy link
Member Author

Bit torn on naming here; and while treeToValue() would be consistent with ObjectMapper/ObjectReader, I think I'll go with readTreeAsValue() instead just because API of DeserializationContext will then group related methods better together.

@cowtowncoder cowtowncoder changed the title Add a convenient way to convert JsonNode to class (like treeToValue()) via DeserializationContext Add DeserializationContext.readTreeAsValue() methods for more convenient conversions for deserializers to use Feb 2, 2021
@cowtowncoder cowtowncoder added this to the 2.13.0 milestone Feb 2, 2021
@jonenst
Copy link
Contributor

jonenst commented Sep 13, 2023

hi @cowtowncoder , so is it possible to write a fully fluent style oneliner to extract one property and bind as pojo ? Otherwise, do you plan to add it ?

mapper.XXreadTree(in).XXget("foo").XXasValue(Pojo.class) // is it possible ?
mapper.treeToValue(mapper.readTree(in).get("foo"), Pojo.class) // not fluent

Thanks in advance

@JooHyukKim
Copy link
Member

It’s been more than two years since completion and there’ve been patches since then. i think at this point there’s more chance to get answers from documentations and resources out on the internet 👍🏼

@cowtowncoder
Copy link
Member Author

@jonenst Issue tracker is not a good place for discussions not directly related to specific issue.

As to your specific example, no, that will not be possible to support -- asValue() would need access to ObjectMapper, for example.

@jonenst
Copy link
Contributor

jonenst commented Sep 15, 2023

Hi,
thank you both for your answer, your time and your hard work on this awesome project.
I posted a message in a github issue because I wasn't just interested in my own problems and project but felt this could help improve jackson (even if only slightly). I saw there was ongoing work on making the API more convenient (e.g. #4056 scheduled for 2.16) so I thought that you might be interested in adding a fluent way to do this.

I chose this issue because it felt like the base of all the related work surrounding this. sorry if that's not appropriate. I have indeed searched extensively the docs and other internet resources (I'm looking at you stackoverflow) for answers regarding this but never found anything better than the first message of this issue:

A very common use pattern for deserializers is to read content as a tree (JsonNode) and then extract properties, either simple scalar values (Strings, numbers, boolean), or to further bind as POJOs via delegation.

I think it's safe to say that people are getting confused by this (for example this top voted answers recommending to create (cheap) configurations objects (writerFor) https://stackoverflow.com/a/39237947/ ), instead of using the simpler treeToValue (I guess because the treeToValue override with java generics was added only later, and the one with a typereference that doesn't require the user to call mapper.constructType even later in 2.16)

I think having a fluent api to do this would reduce the confusion because fluent apis give the feeling that the use case it totally natural and supported. Otherwise, adding it to the docs of jackson in N minutes ( https://github.com/FasterXML/jackson-databind/ ) (maybe in the 3 minutes or the 5 minutes section) would be a good alternative. Because as you said, it's "A very common use pattern"

Finally, I understand that asValue() would need access to ObjectMapper, which is why I suggested new XXmethods returning new types that allow this in mapper.XXreadTree(in).XXget("foo").XXasValue(Pojo.class). Sorry if this was not clear. However of course it's a difficult choice to add new (similar) APIs to make fluent style possible for this use case, because on the one hand it simplifies it, but on the other hand increasing the API surface brings complexity !

If you want me to do anything else on this subject, please feel free to ask, otherwise I'll leave it at that.
Cheers,
Jon

@cowtowncoder
Copy link
Member Author

@jonenst Thank you for your thorough explanation; it makes sense wrt your motivations and background. And I can see how such fluent operations would be convenient if feasible. So I am not against achieving what you suggest at all.

But as to linking JsonNode back to the mapper that created it -- this is fundamentally something I do not want to do, unfortunately. It is not something that was accidentally omitted, or that could be added: I feel strongly that such retention is dangerous. It would also prevent use of shared light-weight value types like BooleanNode for true/false, NullNode (and even MissingNode).
It might be problematic wrt common use case of using nodes with different mappers as well (read as JSON, write as Smile).

There may be other ways to achieve fluent style operations, of course; builder (or such) type that could contain transient reference to mapper as well JsonNode(s).
If so, perhaps there was a way to support more powerful fluent processing.
But I am not sure quite how.

@jonenst
Copy link
Contributor

jonenst commented Oct 1, 2023

@cowtowncoder Hi, I went ahead and wrote #4135 , feel free to take the parts you like from it and discard the rest !
Cheers,
Jon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants