Support single-Map @JsonAnySetter methods - #6075
Conversation
|
Can you specify issue number on test methods? @goutamadwant See neighboring test cases as example |
75278f1 to
41e77d2
Compare
Thanks @JooHyukKim, updated the new test method names to include 4889, matching the neighboring test cases. Let me know if any suggestions. thanks! |
@JsonAnySetter methods
JooHyukKim
left a comment
There was a problem hiding this comment.
We need JavaDocs as well.
Thanks!
| if (anyMethod.getParameterCount() == 1) { | ||
| Class<?> type = anyMethod.getRawParameterType(0); | ||
| if (!Map.class.isAssignableFrom(type)) { | ||
| throw new IllegalArgumentException(String.format( |
There was a problem hiding this comment.
Not sure if we can simply throw error, considering we are doing minor version fix...? 🤔
| throw new UnsupportedOperationException("Cannot call createParameterObject() on " + getClass().getName()); | ||
| } | ||
|
|
||
| public void finish(DeserializationContext ctxt, Object instance) throws JacksonException { } |
There was a problem hiding this comment.
Method name is generic too much.
Can we do both below?
- Change name
- Add JavaDoc
I'm talking about all finish methods
|
Thank you @goutamadwant ! One more thing we need is CLA, from: https://github.com/FasterXML/jackson/blob/main/CLA-jackson-2026.pdf (only needs to be sent once) |
|
Hmmmmh. This is WAY bigger change than I'd expect. I wonder if there is some refactoring that could be done to simplify implementation? |
| public static SettableAnyProperty constructForMapMethod(DeserializationContext ctxt, | ||
| BeanProperty property, | ||
| AnnotatedMember setter, JavaType valueType, | ||
| KeyDeserializer keyDeser, | ||
| ValueDeserializer<Object> valueDeser, TypeDeserializer typeDeser) | ||
| { | ||
| Class<?> mapType = property.getType().getRawClass(); | ||
| if (mapType == Map.class) { | ||
| mapType = LinkedHashMap.class; | ||
| } | ||
| ValueInstantiator vi = JDKValueInstantiators.findStdValueInstantiator(ctxt.getConfig(), mapType); | ||
| return new MapMethodAnyProperty(property, setter, valueType, | ||
| keyDeser, valueDeser, typeDeser, | ||
| vi); | ||
| } |
There was a problem hiding this comment.
💡 Edge Case: Map any-setter with non-instantiable concrete Map fails late
constructForMapMethod only remaps the raw Map.class interface to LinkedHashMap; for other non-instantiable declared types (e.g. SortedMap, Map.Entry-less abstract maps, or a concrete type without a default constructor) findStdValueInstantiator returns null and _createMap throws a DatabindException only at deserialization time when the first unknown property arrives. Consider validating instantiability at introspection/construction time (like other constructForMap* helpers already have this same limitation) so misconfiguration surfaces earlier, and/or mapping common interfaces such as SortedMap/NavigableMap to TreeMap.
Was this helpful? React with 👍 / 👎
There was a problem hiding this comment.
Would be helpful to verify this case
There was a problem hiding this comment.
added a test verifying that an unsupported custom Map type reports the expected DatabindException when an unknown property is encountered.
done @cowtowncoder I emailed the pdf to the provided address. |
|
Thanks for the review @cowtowncoder @JooHyukKim looked at all the comments and pushed a follow up update as below.
The no-unknown-properties behavior remains consistent with existing two-argument any-setters. Support for additional Map interfaces was left out because the same limitation exists for Map-field any-setters and would be better handled separately. let me know if there any other suggestions or comments. thanks! |
JooHyukKim
left a comment
There was a problem hiding this comment.
Added a lil more reviews, hope it helps!
| if (am.getParameterCount() == 1) { | ||
| JavaType paramType = am.getParameterType(0); | ||
| if (!paramType.isMapLikeType()) { | ||
| return ctxt.reportBadDefinition(beanDescRef.getType(), String.format( |
There was a problem hiding this comment.
Is it really possible to reach this code path? If so, this would throw error --backward-incompatible
There was a problem hiding this comment.
nice point @JooHyukKim only one-argument Map methods are now registered as any-setters. Non-Map methods remain regular setters also has a regression test covering this behavior.
| public static SettableAnyProperty constructForMapMethod(DeserializationContext ctxt, | ||
| BeanProperty property, | ||
| AnnotatedMember setter, JavaType valueType, | ||
| KeyDeserializer keyDeser, | ||
| ValueDeserializer<Object> valueDeser, TypeDeserializer typeDeser) | ||
| { | ||
| Class<?> mapType = property.getType().getRawClass(); | ||
| if (mapType == Map.class) { | ||
| mapType = LinkedHashMap.class; | ||
| } | ||
| ValueInstantiator vi = JDKValueInstantiators.findStdValueInstantiator(ctxt.getConfig(), mapType); | ||
| return new MapMethodAnyProperty(property, setter, valueType, | ||
| keyDeser, valueDeser, typeDeser, | ||
| vi); | ||
| } |
There was a problem hiding this comment.
Would be helpful to verify this case
There was a problem hiding this comment.
Just style suggestion so quite subjective but, how about isolate into new _addSetterMethod() method like we have for arg count 0 case?
There was a problem hiding this comment.
Done @JooHyukKim one-argument methods now delegate to _addSetterMethod(), which handles Map any-setter classification before regular setter discovery.
Code Review 👍 Approved with suggestions 2 resolved / 3 findingsAdds support for single-argument Map 💡 Edge Case: Map any-setter with non-instantiable concrete Map fails late📄 src/main/java/tools/jackson/databind/deser/SettableAnyProperty.java:94-108 📄 src/main/java/tools/jackson/databind/deser/SettableAnyProperty.java:462-470
✅ 2 resolved✅ Edge Case: Map any-setter may be skipped in view/external-id paths
✅ Edge Case: Map any-setter method not invoked when no unknown props
🤖 Prompt for agentsOptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
goutamadwant
left a comment
There was a problem hiding this comment.
addressed all the review comments. thanks
Fixes #4889
Root cause
@JsonAnySetterdiscovery handled fields, creator parameters, and two-argument setter methods, but explicitly annotated one-argument methods were still treated as regular setters. That meant a method likesetOther(Map<String, Object> other)was not recognized as an any-setter target.Change
This adds support for explicitly annotated single-argument
Mapany-setter methods. Unknown properties are collected into a map using the existing any-setter key/value deserialization flow, then the annotated method is invoked once with the collected values.Verification
./mvnw -q -Dtest=tools.jackson.databind.deser.AnySetterTest#testJsonAnySetterOnMapMethod test./mvnw -q -Dtest=tools.jackson.databind.deser.AnySetterTest test./mvnw -q -Dtest=tools.jackson.databind.deser.AnySetterTest,tools.jackson.databind.deser.builder.BuilderSimpleTest,tools.jackson.databind.deser.creators.AnySetterForCreator562Test,tools.jackson.databind.introspect.POJOPropertiesCollectorTest test./mvnw -q test./mvnw -q -DskipTests animal-sniffer:checkgit diff --check