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

DateTime serialization should preserve the timezone #43

Closed
jsravn opened this issue Jun 5, 2014 · 16 comments
Closed

DateTime serialization should preserve the timezone #43

jsravn opened this issue Jun 5, 2014 · 16 comments
Milestone

Comments

@jsravn
Copy link

jsravn commented Jun 5, 2014

I guess it has always been like this, but it doesn't make much sense to me. DateTime is basically a LocalDateTime with a DateTimeZone attached. Generally when people are using DateTime they want time zone info, otherwise they would use a LocalDateTime.

I think as a result of this design decision, many people have to implement custom DateTime functionality to deal with time zone.

What jackson should be doing is to write out the time zone id along with the millis for DateTimes.

@cowtowncoder
Copy link
Member

Always open for good contributions; that's how this module gets improved.

@thomaseizinger
Copy link

I have looked into the code of DateTimeSerializer and I think the problem could be solved, if the DEFAULT_FORMAT constant would be changed to no longer include the withZoneUTC() method call.

@cowtowncoder
Copy link
Member

@if12b017 Perhaps, but wouldn't this be backwards-incompatible change? That is, change existing behavior in a way that existing code would start working differently, possibly causing issues.

@thomaseizinger
Copy link

That is true, but anyone who upgrades his libraries to a newer version should be aware of the changes.
There were already some api-breaking changes in the jackson library itself, so I would not see a major drawback here.
You can, of course still preserve the existing behaviour and include a config switch but I would much rather prefer the first option because there is already way to tell jackson in which timezone timestamps should be serialized.
Thats actually only a single line of code to restore the original behaviour.
However, I do not know if the joda module picks up this setting in any way, as I am not familiar with the code in detail.

@guidomedina
Copy link

I think it all boils down to the concept of time, time zone is a meta information, the information itself never changes, which is the time in milliseconds, an instant of time is the same everywhere in the world, it is just the meta info that changes, in fact, by expressing such meta info as UTC you would be losing such meta data and preserving only the instant of time data.

We have dealt with DateTime in Riak using Jackson and we live with such trade off, we have to always recreate the zone using some user information or sort of configuration, I would say storing the zone is not a bad thing at all, in the contrary.

This special case is only valid for DateTime because it is the universal holder for an instant in time with a zone meta in it, if you store things like LocalDate or LocalDateTime you have to have some sort of intelligence of what zone that data was for.

We always disable the feature SerializationFeature.WRITE_DATES_AS_TIMESTAMPS because it is more user friendly/readable and it is very expressive, with such feature disabled the zone can be just expressed as ISO date time which should be the standard and not to default automatically to the UTC String but gather the zone from the (and specially for) DateTime

@guidomedina
Copy link

In the following example we know we set the time zone for Australia (+10:00) but the String stored in Riak is its UTC representation which loses the zone meta data for that DateTime:

    {
      "time": "2014-08-27T03:40:56.000Z",
      "value": "3.46",
      "type": "BATTERY_VOLTAGE"
    }

Which adds the overhead of creating another DateTime instance and that's something I can asure you many developers are doing today DateTime timeAtZone=pojo.getTime().withZone(zone) in order to recover/restore the zone meta information.

@thomaseizinger
Copy link

Yes, I have got the same line in my code several times.
Like you said, timezone is a meta information that gets lost on serialization.
As I mentioned earlier, Jackson already provides a way to set a global timezone for serialization and in my opinion, if no global timezone is set, Jackson should not touch the timezone. Converting the timestamp to UTC by default is a behaviour that has to be documented and is not something you would expect to happen if you are getting in touch with the library.

@rolanddb
Copy link

I'm facing this issue as well. I want to preserve the timezone and don't want to convert to UTC.

Interestingly, on deserializing an ISO formatted DateTime string, you can set mapper.disable(DeserializationFeature.ADJUST_DATES_TO_CONTEXT_TIME_ZONE);
Which avoids the conversion to UTC. If we have that flag as a SerializationFeature as well, it would solve the issue.

@guidomedina
Copy link

I think the default behaviour should be to preserve on the ISO date/time String the DateTime meta-data; meaning, the current time zone offset like yyyy-MM-ddTHH:mm:SS...-/+XX:YY

@tommack
Copy link

tommack commented Aug 27, 2014

One wrinkle is that DateTime can hold a full time zone (e.g. America/Chicago) where as the ISO format will constrain that to an offset which cannot be converted back to a full zone. So, some of the time zone meta data will be lost regardless.

@cowtowncoder
Copy link
Member

Quick note: this question REALLY needs to be discussed on dev mailing list, since it relates to multiple datatypes, not just Joda. Specifically core JDK data types from 1.0, as well as new 1.8 Date API is involved.
Plus not all devs follow updates to all Jackson issues, so @beamerblvd (for example) might not be aware of this discussion.

@dbyron0
Copy link

dbyron0 commented Aug 29, 2014

I did propose #44 to try to deal with this. I do plan to bring it up on the dev list, and I have some changes I'd like to make to that PR....I suppose this is my +1 that this retaining the time zone across de/serialization operations of DateTime is important.

@cowtowncoder
Copy link
Member

Right, I am just saying that I don't want to get a change in that would end up having to be reverted.
And since I am not a domain expert here, so while proposed changes sound reasonable enough to me, I know that details matter. Especially so when changing default handling.

@dbyron0
Copy link

dbyron0 commented Sep 1, 2014

I've now got #44 where I think it's correct at least as far as the time zone details are concerned. I'm not sure it's correct as far as defaults and overriding formats and those kinds of issues. I imagine there's room for improvement even if it is correct.

I've also posted to the dev list (http://markmail.org/message/73aw5vcvsb63b7lt).

@cowtowncoder
Copy link
Member

As per my comment to the PR #44; Jackson 2.6 adds

SerializationFeature.WRITE_DATES_WITH_ZONE_ID

which at least allows enabling/disabling of zone-id inclusion over offset.
But I am not quite sure what the representation here should be.

One possibility here is that proposed format (like "0/UTC") would actually be used when writing is (otherwise) by timestamp (numeric). Conversely, when using string representation, we'd use full ISO-8601(-like) String, followed by suffix, perhaps "/zoneid" if that is common enough standard?

@cowtowncoder cowtowncoder added this to the 2.6.0 milestone Aug 7, 2015
@cowtowncoder
Copy link
Member

As per #44/#66 now implemented for 2.6[.0].

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

7 participants