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

Behaviour of custom date format with regards to the ObjectMapper's default time zone #1661

Closed
brenuart opened this issue Jun 15, 2017 · 7 comments

Comments

@brenuart
Copy link
Contributor

brenuart commented Jun 15, 2017

Tested with Jackson 2.8.8 and 2.8.9.

The general rule with the StdDateFormat with regards to the TimeZone is:

     the TimeZone of the ObjectMapper is used if the JSON input does not contain
     any timezone/offset information.

     If the mapper timezone is set to GMT+1, the input 2000-01-01T00:00:00.000
     (no timezone) will produce a date 2000-01-01T00:00:00.000+01:00

This rule remains valid when the @JsonFormat annotation is used on a property unless it forces an explicit timezeone, in which case it takes precedence.

One would expect the same behavior when the StdDateFormat is replaced by a custom DateFormat on the ObjectMapper. In other words, the timezone of the DateFormat should be of no importance since the ObjectMapper's default is used whenever needed.

However it appears this behaviour is not always respected and depends on whether the mapper has been explicitly configured with a TimeZone or not.

Suppose the following custom date format:

DateFormat df = new SimpleDateFormat("yyyy-MM-dd'X'HH:mm:ss");
df.setTimeZone( TimeZone.getTimeZone("GMT+4") );

Use it on a vanilla ObjectMapper without any further configuration:

ObjectMapper mapper = new ObjectMapper();
mapper.setDateFormat(df);
Date d = mapper.readValue("2000-01-02X04:00:00", Date.class);

This produces a date with GMT+4 although we would expect UTC according to the "rules" described above.

Do a second test and use the same date format on an ObjectMapper whose timezone is explicitly set to UTC. Since UTC is the default, this test should behave as the first...

ObjectMapper mapper = new ObjectMapper();
mapper.setTimeZone(TimeZone.getTimeZone("UTC"));
mapper.setDateFormat(df);
Date d = mapper.readValue("2000-01-02X04:00:00", Date.class);

Now this produces a date in UTC and not GMT+4 anymore.. This is compliant with the rules, but unexpected in the light of the first test.
Do the test once more with GMT+2 instead go UTC and the date will be in GMT+2...

This case is covered by the DateDeserializationTest#testDateUtil_customDateFormat_withoutTZ() test case submitted in PR #1660.

Could this be related to https://github.com/FasterXML/jackson-databind/blob/2.8/src/main/java/com/fasterxml/jackson/databind/cfg/BaseSettings.java#L245-L249 ?

@cowtowncoder
Copy link
Member

cowtowncoder commented Jun 16, 2017

Hmm. I may be wrong, but I think the fundamental question is this: when given a DateFormat, can we trust it to have timezone information to use or not? There is no way (AFAIK) to know if it has been configured or not, so the assumption should be that caller has specified timezone information, and that this should not be changed.
This includes explicit timezone configuration.

Does this make more sense? I remember struggling with this, in trying to figure out how various pieces of information should be combined.

That said, due to complexity of logic it is possible that something may be handled in demonstrably wrong way.

@brenuart
Copy link
Contributor Author

when given a DateFormat, can we trust it to have timezone information to use or not?
No, we cannot trust it.

I think the general rule stated above is correct and must be applied consistently everywhere. This means the TimeZone of the custom DateFormat must be ignored and always forced to whatever BaseSettings.getTimeZone() returns.

This will make the behaviour and the API more consistent and understandable. There will be one single place where the default TZ must be configured: the ObjectMapper.

Furthermore, knowing that Jackson's default TZ is UTC, it is hard to explain someone he will get a different behaviour if he explicitly sets the TZ to UTC himself on the mapper... (that should have no effect).

brenuart added a commit to brenuart/jackson-databind that referenced this issue Jun 16, 2017
In summary, the custom DateFormat always has its TimeZone forced to whatever `Settings.getTimeZone()` will return.
Provide an additional fix in `withTimeZone` to handle the case where `_dateFormat` is null (since this is allowed by `withDateFormat`).
Update test case to reflect the fixed behavior.
@cowtowncoder
Copy link
Member

No: if user feels compelled to specify DateFormat, it better have timezone set too. But there is no way, via API, to see what user did -- it has to be assumed it was properly configured.
But if further calls are made to force timezone, that should indeed override settings of configured DateFormat.

It is possible documentation should be clarified, but at this point further changes seem more likely to confuse things, and lead to more bug reports.

@Martin-Luft
Copy link

I'm not sure if this is the same issue:

Until 2.9:
I set the objectMapper.setTimeZone() and the string 2016-10-30T17:00:00 was interpreted with the given time zone and not as UTC.

Since 2.9:
The string 2016-10-30T17:00:00 is interpreted as UTC (ignoring my set time zone) which breaks my application logic.

@brenuart
Copy link
Contributor Author

The fix proposed in this issue has not been merged (yet). On the other hand, it would affect only those using a custom date format instead of Jackson's StdDateFormat.

I think your issue is similar to #1657

@cowtowncoder
Copy link
Member

@Martin-Wegner it definitely sounds like #1657, which may have made it in 2.9.0.pr4 (not 100% sure, fix is quite close to release time), but is definitely in master.

@cowtowncoder
Copy link
Member

Not sure if and how to proceed here; closing for now.

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

3 participants