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

No attempt to resolve type for AnySetter. #337

Closed
askvortsov-ideas opened this issue Oct 29, 2013 · 8 comments
Closed

No attempt to resolve type for AnySetter. #337

askvortsov-ideas opened this issue Oct 29, 2013 · 8 comments

Comments

@askvortsov-ideas
Copy link

com.fasterxml.jackson.databind.deser.SettableAnyProperty#deserialize does not honor DefaultTypeResolver (and any other) - compare it to com.fasterxml.jackson.databind.deser.SettableBeanProperty#deserialize.
Without type resolving I, personally, cannot create a 'Patch' pojo, that will hold some metadata as bean props and actual data being deserialized through AnySetter.

So, answering a question at the class javadoc for SettableAnyProperty:
Yes, it makes sense to refactor to share some code * with {@link SettableBeanProperty} :)

Until then, how can I override? Extend com.fasterxml.jackson.databind.deser.BeanDeserializerFactory, override its #constructAnySetter to create instances of modified SettableAnyProperty? And modify Mapper's constructor to set a different _deserializationContext?

@askvortsov-ideas
Copy link
Author

Awww, why SettableAnyProperty and its #deserialize are final? Thank OSS, I was able to undertake it, but I would ask to not mark anything final. :)
So I've ended up with adding simple condition

        if (_valueTypeDeserializer != null) {
            return _valueDeserializer.deserializeWithType(jp, ctxt, _valueTypeDeserializer);
        }

to #deserialize calling

typeDeser.forProperty(null)

in the constructor. That is enough for me, because I use as.property type inclusion.

@cowtowncoder
Copy link
Member

I don't know whether it is a good idea to use polymorphic types via any-setter. To me that sounds like asking for trouble. You can handle conversions, if you choose to do so, by using "untyped" approach, pass a Map of String-to-Object mappings.
But I can accept argument that such functionality should be supported if possible. So I will consider this a request to support polymorphic types via any-setter and any-getter.

As to SettableAnyProperty... that is not meant to be sub-classed; it is not an extension point. This is why it is declared as final.
Without marking things final unexpected usage can crop up; although individual cases are always matters of judgment. So I am open to persuasion for specific reason for changing this; but not in 2.3 (we are feature complete).
The problem is that sub-classing is a very fragile way of extending things, making it more difficult to change internals.

@askvortsov-ideas
Copy link
Author

Well, if I'll use 'untyped' approach, I'll just do same work as Jackson can - find all maps (I don't hardcode possible keys), passed to AnySetter, find inclusion property value, determine the class and copy other properties. Such operation can be injected into current process, I believe. Plus it may be useful to allow a better deserialization of some 'primitives' - notably Date`s.
Unfortunately, with the above change there is no fallback to Map, if property is not found. If I'll resolve this, I'll post here.

With regard to final. The main reason I hate it is due to imposed requirement to modify source if some 'patch' is needed. Of course, this changes behavior, and may result in additional complexities while upgrading. But I would rather tick

I acknowledge possible issues caused by this approach
and understand and embrace desolation in fixing those issues...

and so on and so force. :) This usually much more easier then maintenance of modified artifact. And without subclassing, there is a need to override a bunch of classes, because for some reason they all expect concrete class. ;)

@cowtowncoder
Copy link
Member

Understood -- closing of doors saves trouble from some, adds more to others.

I think what will work here is to first see if polymorphic typing can be supported -- I suspect it should, and it is just an oversight on my part.
If that was not to work for some reason, I could open things up. It is possible that I defined class as final as safety precaution, more than actual specific reason.

I hope to be able to check this out between 2.3.0-rc1 (now) and rc2 or final; if so, fix would be in 2.3.0-final.

Thank you for reporting this!

@askvortsov-ideas
Copy link
Author

Ok, thank you. Looking forward to the best databinder becoming better.

cowtowncoder added a commit that referenced this issue Nov 1, 2013
@cowtowncoder
Copy link
Member

Added unit test, I can verify the problem: serialization does add type information properly, but deserializer does not handle it.

@cowtowncoder
Copy link
Member

Was easy fix, will be in 2.3.0-rc2 (if we do that), or 2.3.0 final.

@askvortsov-ideas
Copy link
Author

Ok, thank you.

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

2 participants