Initial Jref deserialization implementation#6014
Conversation
functional interface. Added test case for testing second array entry jref to first entry.
JRefValueDeserializer wrapper for all types of ValueDeserializers (except for Reference types). See JRefModule for definition of JRefValueDeserializerModifier.
tools.jackson.databind.deser.bean.JRefBeanDeserializerTest.testMyStringsJRefPath() to test
collections with jrefs.
"jrefs" deserializationcontext attribute name.
🧪 Code Coverage Report
|
|
Ah. Sorry to be blunt but... from quick look, I don't think I would accept pr based on this approach -- too intrusive, adding overhead for ALL processing, regardless of whether JRef processing wanted or not. |
| @@ -0,0 +1,39 @@ | |||
| package tools.jackson.databind; | |||
|
|
|||
| public class JRefResolveException extends RuntimeException { | |||
There was a problem hiding this comment.
Should be a JacksonException.
There was a problem hiding this comment.
OK I will make this change. There seemed to be use of DatabindException static methods...i.e. from, etc to create and throw appropriate JacksonExceptions. Should JRefResolveException be added to that somehow?
I don't understand what you are talking about as too intrusive. If the JRefModule is not added during ObjectMapper build/creation, then no ValueDeserializerModifiers/JRefValueDeserializers are created, no checking for Jrefs is done and so no JRefPaths are detected or created during parsing. No extra mem nor method calls are necessary. The only thing that is done is to call ValueDeserializer.setWithJRef(ctxt, value, setterfunction) rather than setting the value in the parent context. setWithJRef adds a JRefPath instanceof test...but since no JRefPaths will be created, the instanceof test will always fail, and so the setter function will be immediately called (not deferred) to set the value synchronously...i.e. I don't see a single instanceof check and calling a setterfunction (instead of executing the block directly) as overly intrusive...it won't have large runtime cost relative to executing the block directly or even an inner class...as this assignment has to be/is done. I haven't looked at all the details, but it appears through a casual search that modern jits can optimize repeated lambda/functional interface calls I could also add a context config option (support jref reference resolution or not) to guard the call to setWithJRef and so avoid calling setWithJRef at all when jref resolution is not enabled. |
|
Another option for guarding calling setWithJRef would be to check whether jref module is active and only call setWithJRef if it is...otherwise do value setting in previous manner. |
I want to follow up on the question of intrusiveness wrt the setWithJRef method impl and it's usage. First...I decided to have a separate setWithJRef method...rather than implementing separately inline these two operations and if the value is not a JRefPath b) Call function/block that actually does the setting (if normal deserialization/no JRefPath) because essentially the same logic has to be used by multiple different ValueDeserializers..in order to allow the usage of jrefs for all different types (each with a distinct way of setting a deserialized value...e.g. Map, Collection, Beans (and probably others). Since providing this impl, I've realized that a) The if (value instanceof JRefPath) conditional could be changed or enhanced to (e.g.) also check for whether the JRefModule is in present/active, or an ctxt attribute is set, or a feature is set/not set or whatever. b) The SetterFunction argument (and the setWithJRef method) could be removed entirely...and the conditional and code block execution could be inline. The cost of inline being a couple of more lines of similar/same code in multiple 'set' locations (which seemed to me to be more intrusive than setWithJRef implemented once). My point is that we can enhance a to support other conditionals based upon config (module, feature, option, etc) and/or refactor as per b) if the runtime cost as implemented seems too high. Given the dynamic jit, though...my belief is that the runtime cost of a and b/setter function is going to be very low/non-intrusive relative to other mechanisms (e.g. throwing/catching exception, more general 'setter customization'). |
🧪 Code Coverage Report
|
|
Quick note: right, all Module-provided handlers are fine; only registered as-needed. No concern there. Afterburner module (https://github.com/FasterXML/jackson-modules-base/) does something like this for |
I'm fine/ok with making things less dynamic (below), but I don't really understand the problem with creating/passing lamba/functional interface. Are you concerned about runtime costs for use of lambda, or something else? One advantage (and why I put it there to begin with) is that is reduces the amount of code to handle all the parent ValueDeserializer setting as discussed below.
Yes...it can...but it does mean that the code block for doing the actual set will have to be copied for both cases...i.e. JRefPath and the normal path. That could create a maintenance issue having two separate code blocks for every 'set'. That's why I introduced the SetterFunction to begin with as initially I had two copies of the same block...1 for the regular/no jref case and then for the deferred/JRefResolver case...which usually should be exactly the same....thus setWithJRef.
I'll look at it, thanks. FWIW, I'm happy to use what's already present in Jackson...rather than e.g. SetterFunction so all such references appreciated. |
2 participants
This pr provides a first cut implementation of JRef handling for Jackson deseriailzation. For addressing issue #5920
The basic strategy is to
a) extend all ValueDeserializers so that they can detect jrefs the form
{ "$ref": "#/some/path" }during object deserialization, and when detected defer the setting of the referenced value until after the root and the rest of the object graph has been deserialized.
b) Once the root is available, the path can then be used to resolve/lookup the referenced value, and then used to set a target to that value. As jrefs are local-only, the path must be relative to the root.
This commit adds the following classes. Currently in tools.jackson.databind package.
JRefModule - A subclass of SimpleModule that uses a ValueDeserializerModifier to create wrapper instances (of type JRefModule.JRefValueDeserializer for all appropriate types of ValueDeserializer.
JRefPath instances of this class are returned by JRefValueDeserializers when found on the JsonParser stream.
SetterFunction - A functional interface defining a block responsible for setting a target to a value.
In the each deserialization context (e.g. MethodProperty for Bean property setting ) the handling of the Object deserialization result is passed to ValueDeserializer.setWithJRef which either
a) If the value parameter is a JRefPath instance - a JRefResolver is created and added to the deserialization context. Once a root object is available...see ObjectMapper...JRefResolver.resolve(root) can be called, and resolve looks up a value given the JRefPath relative to the now available root object. That value is then used to set a to the value via a call to the SetterFunction.
XOR
b) If the value parameter is not a JRefPath instance - setWithJRef immediately calls the SetterFunction.set method with the value parameter. This usual case allows the enclosing type to define the setting block via a single SetterFunction, that is used either immediately with the given value or deferred until JRefResolver.resolve(root) can be called to resolve and set.
JRefResolveException runtime exception class for jref resolution failures.
Test Cases for Deserializing JRef references:
JRefBeanDeserializerTests - Tests of the JRef deserialization and resolution. All current test cases are passing. More need to be added to support other Jackson deserialization features.
Note: // XXX JREF is used as a comment in the existing Jackson classes that have had code changes.