-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fixed problem in CollectionDeserializer
where _nullProvider
was not used if it was not JsonToken.VALUE_NULL
#5140
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
Conversation
src/main/java/com/fasterxml/jackson/databind/deser/std/CollectionDeserializer.java
Outdated
Show resolved
Hide resolved
src/main/java/com/fasterxml/jackson/databind/deser/std/CollectionDeserializer.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the name, placement and content of the test appropriate?
The following test was referenced for name and placement.
JooHyukKim@51a92b9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should go next to CollectionDeserTest
in com.fasterxml.jackson.databind.deser.jdk
. Name is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
e4941a5
@Test | ||
public void nullsFailTest() { | ||
ObjectMapper mapper = new ObjectMapper(); | ||
mapper.configOverride(List.class).setSetterInfo(JsonSetter.Value.forContentNulls(Nulls.FAIL)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use JsonMapper.builder()
approach so it merges more easily into 3.x (3.x has immutable ObjectMapper
).
Something like
mapper = JsonMapper.builder()
.defaultSetterInfo(JsonSetter.Value.forContentNulls(Nulls.FAIL))
.build();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
93e1a21
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I am sorry.
There was a mistake and I have made an additional correction.
88bc7e4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
np, thanks!
src/test/java/com/fasterxml/jackson/databind/deser/std/CollectionDeserializer5139Test.java
Outdated
Show resolved
Hide resolved
src/main/java/com/fasterxml/jackson/databind/deser/std/CollectionDeserializer.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
// _skipNullValues is checked by _tryToAddNull. | ||
if (value == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part can be moved within previous block, after value = _nullProvider.getNullValue(ctxt);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
aa4dbab
value = _deserializeRawContentValue(p, ctxt); | ||
} | ||
|
||
if (value == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, can move to inside block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
aa4dbab
* This method only performs deserialization and does not consider _skipNullValues, _nullProvider, etc. | ||
* @since 2.19.1 | ||
*/ | ||
protected Object _deserializeRawContentValue( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method makes sense but I think I'll rename it and can then probably merge
Excellent work as usual. Thank you @k163377 -- merged in for 2.19(.1), will merge to 3.x Bit too risky to backport in 2.18, unfortunately, I think. |
Thank you very much. |
Is there an estimate of when 2.19.1 might be released? We are eagerly awaiting for this fix to be released. Thanks! |
@DavidRigglemanININ Unfortunately due to effort involved (3-4 hours for full patch release) it'll probably be couple of weeks yet. I need to find time to do that (not being part of my day job). |
Yes, with lots going on at the moment, you might want to use and overwrite with latest jar? (not an offical release, but with fix you need. Using Gradle or Maven I guess. --In case you re urgent cc @DavidRigglemanININ . |
I can wait a few weeks. It's not that urgent as we've been living for the pain for quite a while now (as we couldn't use the strictNullChecks on the kotlin module side due to performance issues). It's just been a pain point for us over the years that I'm really excited to finally get behind us as you can probably tell :). Thanks for all your hard work on this project! |
@DavidRigglemanININ Thank you! With Sonatype's aggressive push to shut down OSSRH I may end up actually making release earlier, although first needing to spend a bit of time on migrating publishing to use Sonatype Central Portal. We'll see. |
SSIA
fix for #5139
It was a bit complicated, so I apologize if there are any problems with the modifications.