Fix #1604 for 2.8.11 #1800
Fix #1604 for 2.8.11 #1800
Conversation
| JavaType binding = baseType.containedType(bindingPosition); | ||
| // While the type binding is nested with exactly one inner binding, AND the subtype's definition | ||
| // itself already implies the outermost binding, then... | ||
| while (binding.containedTypeCount() == 1 && _subtypeImpliesBinding(baseType, binding, bindingPosition, subclass)) { |
There was a problem hiding this comment.
This new code path is only activated when a top-level binding type is, itself, bound in a nested fashion to exactly one "inner" type
There was a problem hiding this comment.
It just so happens that this can be done for each type binding in sequence -- no combinatorics involved.
|
|
||
| JavaType[] wildcards = new JavaType[subclass.getTypeParameters().length]; | ||
| Arrays.fill(wildcards, CORE_TYPE_OBJECT); | ||
| JavaType wildcardedSubclass = _fromClass( |
There was a problem hiding this comment.
Maybe wildcardedSubclass isn't the best name for this. Basically, it's a JavaType representation of subclass that's equivalently fully-erased.
| @@ -480,23 +480,82 @@ private TypeBindings _bindingsForSubtype(JavaType baseType, int typeParamCount, | |||
| // (hopefully passing null Class for root is ok) | |||
| int baseCount = baseType.containedTypeCount(); | |||
| if (baseCount == typeParamCount) { | |||
| if (typeParamCount == 1) { | |||
There was a problem hiding this comment.
I split the difference by removing the two inlined cases and using the more optimal array-based invocation for the general case. Just did this for code clarity.
|
I am still not convinced type information was not lost, especially for deserialization case. I have read over patch, but haven't yet quite grokked the logic. Type binding handling gets my brain in a knot so I am not surprised by this, nor does it mean code is not understandable. :) Also, yeah, let's target 2.8. While I don't know if there will be 2.8.11 it is theoretically possible; plus code in this area hasn't changed so it should be simple merge. And while there's risk of regression in a way it's less of a problem this late in patch process when few developers upgrade to latest patch any more (2.9 being out). |
|
👍
…On Wed, Oct 18, 2017 at 2:23 PM Tatu Saloranta ***@***.***> wrote:
I am still not convinced type information was not lost, especially for
deserialization case.
Problem is that declared basetype often has generic bindings (like, say,
List<POJO>), whereas subtype always comes either from runtime type
(serialization) or from annotations (ser/deser -- although mostly for
deser). Latter is the case for polymorphic subtypes as well as
"deserialize-as" annotations. The reason why it is not likely to be
relevant for serialization is that Jackson chooses to use runtime type for
leaf values (unlike, say, JAXB where it is always static type), so even if
we did lose declared type bindings on our way, leaf serializer is located
based on actual instance.
So having type of List<?> does not preclude serialization of actual value.
I have read over patch, but haven't yet quite grokked the logic. Type
binding handling gets my brain in a knot so I am not surprised by this, nor
does it mean code is not understandable. :)
I will keep on reading!
Also, yeah, let's target 2.8. While I don't know if there will be 2.8.11
it is theoretically possible; plus code in this area hasn't changed so it
should be simple merge. And while there's risk of regression in a way it's
less of a problem this late in patch process when few developers upgrade to
latest patch any more (2.9 being out).
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1800 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABIEbdF_OzIEQYX4tIu6bYjijSbIYPOpks5stkG5gaJpZM4P92xC>
.
|
|
Added some more tests to show cases not covered (new test |
Does this code get exercised for the deserialization case? It looks like it's specific to taking an erased |
|
Ok, based on conversations here, code adapted from One potentially new concern is that users find cases that have not failed (and may appear as working), but use types that are not structurally compatible: code now performs validation on compatibility of generic type bindings. I don't think it is likely common problem, but JDK/JVM and Type Erasure do allow some cases, since only assignment compatibility of type-erased types (that is, classes, not included type variable bindings) is checked by JVM. Thank you for your help here @epollan -- I don't think I would have dug deep enough without your input and suggestions! Fix will be in 2.8.11 (if released), and 2.9.3 for sure. |
|
Good to hear, @cowtowncoder. Thanks to @wlp1979 for nailing down the repro case. edit: see the commit cross-referenced in the bug. |
@cowtowncoder Here's my proposed fix for #1604. I have, selfishly, targeted the 2.8 maintenance branch, hoping to get this into a 2.8.11 release (that appears to be the next 2.8.x version given the tag sequence).
The more I thought about this, the more I came to believe that your concern about loss of type binding information isn't warranted. I say that b/c I think when sub-types constrain the type parameterization of their super-types via their inheritance specification, the type information is intrinsic to the class itself, and generic type bindings aren't necessary to build full-fidelity serialization configuration.
I eliminated a bit of inlining for code clarity (the arg count 1 & 2 special branches to avoid the array allocation). I was careful to adapt the new code to the
TypeBindings.create()overloads' affinity towards arrays, so hopefully that offsets the hand-optimization removal ;)I moved the failing test you had already seeded in the 2.9 codebase over into the
typepackage, updated it to assert the expected JSON, and adapted it to the 2.8.xobjectMapper()accessor in the base test class.Thoughts? Any test cases missing?