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

Regression: DateConversionError in 2.6.x (2.5.4 works fine) #939

Closed
anpieber opened this issue Sep 22, 2015 · 10 comments
Closed

Regression: DateConversionError in 2.6.x (2.5.4 works fine) #939

anpieber opened this issue Sep 22, 2015 · 10 comments
Milestone

Comments

@anpieber
Copy link

If you try to convert a Date into a String it reduces it by one (while this works in 2.5.4). If you try to convert 2015-01-02 Date to String it prints out 2015-01-01. This bug is kind of critical I assume. I've created the most primitive test case showing the problem: https://github.com/anpieber/jackson-regression . If you downgrade the version to 2.5.4 in the pom you'll see that it works there (test is green).

@cowtowncoder
Copy link
Member

First of all, thank you for reporting possible issue.

Unfortunately I can not yet reproduce the issue, after fixing a small issue with test: it seems to have an extra space before String date value. But I suspect this is because my default TimeZone (PST) differs from what you have, and that keeps actual date serialization the same.

I can not yet determine whether there is a bug or not; the problem is with lack of time-zone information with SimpleDateFormat, and how that affects end-to-end processing. Ideally given example should work, given that you have not changed settings, and although use of timezone-less formats is dangerous in itself (JDK's java.util.Date and java.util.Calendar always have both date and time parts, as well as associated TimeZone), it would seem like here things should still work the way you expect.

So I will try to dig bit deeper to understand where exactly timezone changes.

There are things you can do to work around the issue. Specifically, I think that setting SimpleDateFormat.setTimeZone() to same value as what ObjectMapper has should synchronize handling. This probably makes sense at any rate.
2.6 does change the handling so that TimeZone format has is NOT overriding by context TimeZone ObjectMapper has: this prevents accidental overrides for cases where caller has specifically set the time zone.

@anpieber
Copy link
Author

Thank you very, very much for your fast answer!

Actually you're right, of course it is about the timezone... I've updated my test case at https://github.com/anpieber/jackson-regression accordingly. Using format.setTimeZone(TimeZone.getTimeZone("UTC")); works like a charm while using format.setTimeZone(TimeZone.getTimeZone("Europe/Vienna")); fails.

While your explanation does make perfect sense, and the workaround is quite simple, I still think that this is not the behavior most ppl are expecting. Especially since the behavior for this changed in a regular minor release.

Kind regards,
Andreas

@cowtowncoder
Copy link
Member

I agree this is not ideal, and I hope to see what causes it. So thank you for reporting the difference.

@cowtowncoder
Copy link
Member

Actually, thinking about this more, I am not sure this is a bug. The problem is this: java.util.Date instances actually do NOT have a TimeZone at all. So what happens when you do date.toString() is that a default format with default system timezone is constructed and used. This is also why you should never really use toString() of Date instances -- you have no control.

You can see this for example by printing date using format from the test itself: it will print the original date.

@anpieber
Copy link
Author

Actually I'm with you and we're feeling for quite some time now that we really should change all dates transmitted in JSON to a format including the timezone...

Independently, I think the text put into Jackson should come out afterwards again; especially if the same converter is/should be used both times. Shouldn't jackson simply use the provided format instead of tostring for the date? Wouldn't this solve the problem?

@cowtowncoder
Copy link
Member

@anpieber Hmmh. Jackson should boe not using toString() for anything (rather it uses DateFormat) so it is actually odd that value changes. I thought test was using toString(), but looking at again it is not.
I'll try to see if I can I can make sense of it; perhaps TimeZone in use between serialization and deserialization (via DateFormat) differs for some reason.
It certainly sounds as if something somewhere (within Jackson) was still using system default timezone for some part of processing.

Not related to this, it may make sense to use Joda library's DateTime, as it contains timezone as part of the value, not just timestamp value, if timezone value matters.

@cowtowncoder
Copy link
Member

One minor comment on test methods: conversationOfTextToDateAndBackShouldWorkForViennaTimezone() is failing because GregorianCalendar is being constructed with system default timezone. So that makes sense.

@cowtowncoder
Copy link
Member

Ah ha! I found the issue; on serialization side, formatter's TimeZone is indeed overridden by contextual default TimeZone that Jackson uses (GMT); but not so with deserializer, where timezone of formatter is used as-is. I'll fix that by changing behavior of serializer to leave tz as is.

cowtowncoder added a commit that referenced this issue Sep 26, 2015
@cowtowncoder cowtowncoder modified the milestones: 2.2, 2.6.3 Sep 27, 2015
@anpieber
Copy link
Author

Hey,

Thank you very, very much!

Just some comments to the commit referenced in this issue.

  1. My name is Andreas Pieber not Andreas Piebe :)
  2. Dont you want to include at least the tests here, avoiding future regressions? https://github.com/anpieber/jackson-regression/blob/master/src/test/java/test/AppTest.java

Ad your comment at conversationOfTextToDateAndBackShouldWorkForViennaTimezone: those tests work as expected; only the notWorkingDateTest was failing.

Ad Joda: We're already using Joda for most parts; there're just "some" legacy parts of the application still dependent on Date...

@cowtowncoder
Copy link
Member

@anpieber Sorry about typo for your last name!

On unit tests: yes, ideally. Will try to add something; will just need to ensure a fair test wrt setting default TimeZone for mapper, and whether to include timezone for formatter or not. Thank you for suggestion!

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

No branches or pull requests

2 participants