Allow @JsonAnySetter on Creator constructors #4366
Conversation
This reverts commit 94540f8.
| fieldType = resolveMemberAndTypeAnnotations(ctxt, mutator, fieldType); | ||
| keyType = fieldType.getKeyType(); | ||
| valueType = fieldType.getContentType(); | ||
| prop = new BeanProperty.Std(PropertyName.construct("stuff"), |
There was a problem hiding this comment.
Shouldnt hard code constructor param name
| AnnotationIntrospector ai = _context.getAnnotationIntrospector(); | ||
| for (int i = 0; i < props.length; i++) { | ||
| SettableBeanProperty prop = props[i]; | ||
| AnnotatedMember member = prop.getMember(); | ||
| if (member == null) { | ||
| continue; | ||
| } | ||
| Boolean hasAnySetter = ai.hasAnySetter(member); | ||
| if (hasAnySetter == null || !hasAnySetter) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Same as '_findCreatorPropWithAnySetter()' method above. Wonder if we can add this method to CreatorProperty class?
| _validateNamedPropertyParameter(ctxt, beanDesc, candidate, i, | ||
| // [databind#562] Any setter can be used... | ||
| Boolean hasAnySetter = ctxt.getAnnotationIntrospector().hasAnySetter(param); | ||
| if (hasAnySetter != null && hasAnySetter) { |
There was a problem hiding this comment.
or if (Boolean.TRUE.equals(hasAnySetter))
|
|
||
| // [databind#562]: Respect @JsonAnySetter in @JsonCreator | ||
| // check if we have anySetter Param | ||
| AnnotationIntrospector ai = _context.getAnnotationIntrospector(); |
There was a problem hiding this comment.
Alas, this is not acceptable from performance perspective: all annotation introspection must occur when constructing deserializers and related handlers. It should be possible to change handling to keep track of information about existence of "any setter", instead of trying to introspect for every deserialization call.
|
Impressive. I am bit hesitant to consider this as one more thing before work on Property Introspection rewrite, but no harm in figuring out how this could work. Added some notes. |
I concur. And also in the issue #562, this feature is to get included in 3.x. So I think we can safely consider this feature only after Property Introspection rewrite.
Thankssss! I will keep them updated |
This reverts commit 6a892f8.
JooHyukKim
changed the title
src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java
Outdated
Show resolved
Hide resolved
src/main/java/com/fasterxml/jackson/databind/deser/BeanDeserializerFactory.java
Outdated
Show resolved
Hide resolved
src/main/java/com/fasterxml/jackson/databind/deser/CreatorProperty.java
Outdated
Show resolved
Hide resolved
src/main/java/com/fasterxml/jackson/databind/deser/CreatorProperty.java
Outdated
Show resolved
Hide resolved
src/main/java/com/fasterxml/jackson/databind/deser/impl/PropertyValueBuffer.java
Outdated
Show resolved
Hide resolved
|
Cool, this is much improved. I'll make tiny changes, wrt suggestions I added. |
| protected Map<Object, Object> initMap(DeserializationContext ctxt) | ||
| throws IOException | ||
| { | ||
| if (_valueInstantiator == null) { |
There was a problem hiding this comment.
It seems to me that this really should be caught during construction and never get this far?
There was a problem hiding this comment.
Moved the check accordingly.
|
@JooHyukKim Ok, I made a few changes that I think help, including validation that only one "any-setter" is specified. I was also wondering if it wouldn't make sense to add knowledge of "any-setter" creator property directly into |
Do you mean do the
Placeholder? 🤔 Could u elaborate a bit more? |
Right.
What I mean is to test to see that if this setting is enabled, it will not cause failure since property that represents "any-setter" has no associated value. |
Add quick check before iteration start
cc/ @cowtowncoder Implemented as suggested in commit 8612433. Apologies in advance for formatter taking up large portion. IMO, this new version seems more appropriate in that we don't handle values twice --first buffering in PropertyValueBuffer then transfer to map for AnySetter parameter. |
resolves #562