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

Fix wrapped array hanlding wrt null by StdDeserializer #4844

Merged
merged 4 commits into from
Dec 12, 2024

Conversation

glorrian
Copy link
Contributor

@glorrian glorrian commented Dec 9, 2024

Motivation: We encountered a bug in Micronaut Framework, when json parsing was rewritten so that any value is wrapped in an array(micronaut-projects/micronaut-core#10070) for the convenience of parsing multi values with one key, then the behavior when parsing null values began to throw an error, because jackson does not handle VALUE_NULL cases separately when parsing from a wrapped array. this pr is needed to fix this.

fix issue: micronaut-projects/micronaut-core#11420

Comment on lines 98 to 100
public static class StringWrapper {
public String value;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

declarations should go top

Comment on lines 86 to 87
public void testDeserializeArrayWithNullElement() throws IOException {
String json = "{\"value\": [null]}";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting + Exception is sort of convention now

Suggested change
public void testDeserializeArrayWithNullElement() throws IOException {
String json = "{\"value\": [null]}";
public void testDeserializeArrayWithNullElement()
throws Exception
{
String json = "{\"value\": [null]}";

@JooHyukKim
Copy link
Member

Minor suggestions, looks clean tho

@cowtowncoder
Copy link
Member

Thank you @glorrian! This is fix is correct: deserialize() is not expected to handle null values, caller should.

Aside from suggestions @JooHyukKim gave, one and only process thing we have is that we need to get CLA:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

(unless you have earlier sent one; CLA only needs to be sent once and is good for all contributions)

The easiest way is usually to print the doc, fill & sign, scan/photo, email to cla at fasterxml dot com.

Once I get it (or find existing one) I'm happy to merge this. I can also backport this in 2.18(.3) since it seems pretty safe to me.

Thank you again!

Copy link
Member

@JooHyukKim JooHyukKim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comment wrt import ordering, thanks!

@glorrian
Copy link
Contributor Author

hi, thank y'all for review, i sent my cla to clas at fasterxml dot com from glorrian55 at yandex dot ru, hope you'll receive it.

@glorrian
Copy link
Contributor Author

@JooHyukKim all your suggestions is completed

@JooHyukKim
Copy link
Member

LGMT! Thank u @glorrian

@yawkat
Copy link
Member

yawkat commented Dec 10, 2024

Thanks for tracking this down!

@cowtowncoder
Copy link
Member

@glorrian Unfortunately I don't see CLA. If you used "clas", it needs to be "cla", so:

cla@fasterxml.com

(or info@)

Could you try re-sending?

@glorrian
Copy link
Contributor Author

glorrian commented Dec 11, 2024

@cowtowncoder i try to send from different mail to info and cla both, check it pls
it gonna be mail from sherbakov05.bkmail.ru@gmail.com

@cowtowncoder cowtowncoder added the cla-received PR already covered by CLA (optional label) label Dec 12, 2024
@cowtowncoder
Copy link
Member

@cowtowncoder i try to send from different mail to info and cla both, check it pls it gonna be mail from sherbakov05.bkmail.ru@gmail.com

@glorrian Got it now, thanks!

@cowtowncoder cowtowncoder changed the title Fix wrapped array with null parsing into StdDeser Fix wrapped array hanlding wrt null by StdDeserializer Dec 12, 2024
@cowtowncoder cowtowncoder merged commit 8cfa8ba into FasterXML:2.19 Dec 12, 2024
6 checks passed
cowtowncoder added a commit that referenced this pull request Dec 12, 2024
@cowtowncoder cowtowncoder added this to the 2.18.3 milestone Dec 12, 2024
@cowtowncoder
Copy link
Member

Merged, thank you @glorrian for the report & fix!

@glorrian glorrian deleted the wrapped-array-with-null branch December 13, 2024 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-received PR already covered by CLA (optional label)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants