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

Issue #71 fix #72

Merged
merged 1 commit into from
Sep 15, 2015
Merged

Issue #71 fix #72

merged 1 commit into from
Sep 15, 2015

Conversation

nezda
Copy link
Contributor

@nezda nezda commented Aug 26, 2015

[#71] use configured TimeZone when deserializing instants (longs) into LocalDate/LocalDateTime

@cowtowncoder
Copy link
Member

Quick question: do you think this is safe enough fix to include in 2.6.2 patch? Or should it go in master for 2.7?

@nezda
Copy link
Contributor Author

nezda commented Sep 9, 2015

I believe it is safe enough to include in a patch: copied structure from elsewhere to access ctxt.getTimeZone() and has tests.

@cowtowncoder
Copy link
Member

I don't doubt the code works in that sense, but was more wondering whether users could face problems with it. LocalDate is supposed not to have specific time zone, so it may well not matter (that is, usage that assumes specific tz would seem suspect).

But assuming this is fine for either 2.6 or 2.7, one thing I need for merging is a CLA (apologies if I have received one earlier; I tried to find one). It's here:

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

and what is usually done is that contributor prints, fills & signs it, scans and emails to info at fasterxml dot com. Once we receive it, I can merge the patch.
If you could do that, I'd be happy to merge the patch appropriately.

@nezda
Copy link
Contributor Author

nezda commented Sep 9, 2015

I see -- it is possible someone deserializing a LocalDate or LocalDateTime encoded as a long (rare?) might be surprised by this change but this change gives them the capability to work around it easily too by setting their context TimeZone to their local timezone, although they'd been depending on a bug. I'd love to see the change in 2.6.2.

Just sent signed CLA.

@cowtowncoder
Copy link
Member

Thanks, CLA received and handled.

I sent a note on dev list, and would like to see if anyone else has opinion. I know there are couple of developers more knowledgeable wrt date/time, Joda, Java 8 date/time than I am, hoping they would chime in.

I agree in that combination of long value, local date, should mean this is reasonably safe change.
But at the same time use of Joda is so wide that even if only minority uses that combination it can affect many end users. So better safe than sorry.

@cowtowncoder
Copy link
Member

Ok, I think this makes, and should go in 2.6[.2].

cowtowncoder added a commit that referenced this pull request Sep 15, 2015
@cowtowncoder cowtowncoder merged commit 7939d83 into FasterXML:master Sep 15, 2015
@cowtowncoder cowtowncoder added this to the 2.6.2 milestone Sep 15, 2015
@cowtowncoder
Copy link
Member

Merged; will see if I can do any cleanup (wrt access of tz), release 2.6.2.

@nezda
Copy link
Contributor Author

nezda commented Sep 15, 2015

Awesome!

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

Successfully merging this pull request may close these issues.

2 participants