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

Preserve the original component type in merging to the array #4117

Closed
wants to merge 1 commit into from
Closed

Preserve the original component type in merging to the array #4117

wants to merge 1 commit into from

Conversation

yurkom
Copy link
Contributor

@yurkom yurkom commented Sep 18, 2023

It fixes an exception:

Exception in thread "main" com.fasterxml.jackson.databind.JsonMappingException: 
Problem deserializing property 'date1' (
expected type: [array type, component type: [simple type, class java.util.Date]];
actual type: `java.lang.Object[]`),
problem: Can not set [Ljava.util.Date; field com.example.Data.dates to [Ljava.lang.Object; 
(through reference chain: com.example.Data["date2"])

@yurkom
Copy link
Contributor Author

yurkom commented Sep 18, 2023

It would be nice if this fix will be backported to 2.14 and 2.15 also.

@cowtowncoder cowtowncoder added cla-needed PR looks good (although may also require code review), but CLA needed from submitter 2.16 Issues planned for 2.16 labels Sep 18, 2023
@cowtowncoder
Copy link
Member

Looks good, thank you for contributing this @yurkom !

One thing before merging (and considering where to backport): unless we have a CLA from you for earlier PRs (if so, please LMK), we'd need one now:

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

(this is only needed once before the first contribution to any Jackson project)

and is usually done by printing doc, filling & signing, scan/photo, email to info at fasterxml dot com.

Once this is received I can merge PR.

As to backporting, since this looks quite safe it could go in 2.15 for sure (for which there will probably eventually be a patch release); I could merge it in 2.14 but it's not clear if or when a patch might be released.
Still, could add there in case of eventual release.

@JooHyukKim
Copy link
Member

It would be nice if this fix will be backported to 2.14 and 2.15 also

Wrt backporting, how about we start a new PR based on 2.14 or 2.15 (whichever we agree on)? So we can just forward-merge naturally.

@cowtowncoder
Copy link
Member

Yes, re-creating PR to target 2.14 would be nice. But I can alternatively just cherry-pick this one too if that's too much hassle.
I think going to 2.14 is fine here.

I can merge this in 2 days (right now can't commit but can comment :) )

@JooHyukKim
Copy link
Member

If it's not too much hassle for you @yurkom, following steps will get things done 👍🏼👍🏼

  1. Submit CLA (doesn't bind to any PR)
  2. Close this PR
  3. Create a new branch based on latest 2.14
  4. Apply code from this PR
  5. Submit PR

@yurkom
Copy link
Contributor Author

yurkom commented Sep 19, 2023

Hi,
I have submitted the CLA and created 2 pull requests for 2.14 (#4121) and 2.15 (#4120).

@cowtowncoder cowtowncoder removed the cla-needed PR looks good (although may also require code review), but CLA needed from submitter label Sep 20, 2023
@cowtowncoder
Copy link
Member

Hi, I have submitted the CLA and created 2 pull requests for 2.14 (#4121) and 2.15 (#4120).

Thank you; apologies for not mentioning that it is enough to create just one (oldest branch, 2.14) and we can merge forward.

I will merge close this issue and leave 2.14 pr; hope to merge some time tomorrow or day after (right now don't have commit access due traveling)

@cowtowncoder
Copy link
Member

Replaced by #4121

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.16 Issues planned for 2.16
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants