PR to resolve #13 - ISO 8601 serialization of types ZonedDateTime and OffsetDateTime

[zkiss]

A solution proposal for issue #13.

[cowtowncoder]

@zkiss Thanks! I'll have to get a second opinion from Nick, just to make sure; sounds legit to me.

@beamerblvd What do you think?

[zkiss]

No worries! On a sidenote, mvn clean verify was failing on current master due to some warnings and the fact that -Werror is enabled in maven. Was wondering how you guys do a release...

[cowtowncoder]

Released using maven. Maybe master has issues due to changes in 2.5.0-SNAPSHOT for databind?

[zkiss]

I have just noticed that DateTimeFormatter.ISO_ZONED_DATE_TIME actually produces a non-ISO output, but an extension of it.

Since the issue is about that the library does not produce ISO compatible date strings, I have changed the ZonedDateTimeSerializer to use DateTimeFormatter.ISO_OFFSET_DATE_TIME instead.

I am a bit unsure about this change though, as it leads to loss of data - the time zone information. And also to failing tests. I am afraid it might be impossible to convert ZonedDateTime in a strictly ISO string, which is probably why the formatter DateTimeFormatter.ISO_ZONED_DATE_TIME was created.

My question is what would be the preferred approach here - is the aim of this module to produce ISO compatible date strings or to keep the data and break ISO compatibility?

Personally I would go for ISO serialization and rely on ZonedDateTime.parse (essentially treating it as an OffsetDateTime) - I think this behaviour is expected by most people and provides compatibility with other libraries as well. Also it would be counter intuitive to 'ban' ZonedDateTime from the list of ISO-serialized temporal objects.

This PR cannot be merged in the current state.

[zkiss]

I have chosen to go for ISO compliant serialization and change the expectations in the tests. In cases where a location specific timezone was expected in the deserialized object, the initial conditions were changed to use fixed offset based ZoneId objects. This is because what was stated in the previous comment; ISO compliant date strings do not encode the location of the time zone, only the offset.

This PR now can be merged and contains what I think most people would consider an acceptable ISO compliant behaviour.

[zkiss]

@cowtowncoder, @beamerblvd, just pinging you guys to have a look, maybe you've missed it.

[cowtowncoder]

I haven't done anything on this, but in absence of concerns, I could merge it for inclusion in 2.6, now that 2.5 is safely out.

[beamerblvd]

I'll take a look at this tonight and provide feedback.

Nick

Sent from my iPhone, so please forgive brief replies and frequent typos

On Jan 6, 2015, at 11:26, Tatu Saloranta [email protected] wrote:

I haven't done anything on this, but in absence of concerns, I could merge it for inclusion in 2.6, now that 2.5 is safely out.

—
Reply to this email directly or view it on GitHub.

[jontejj]

Did you take a look at it? :)

[zkiss]

There was some discussion regarding this PR in the issue: https://github.com/FasterXML/jackson-datatype-jsr310/issues/13

We need to make a decision if we still want to merge this PR. A solution to the issue is desired though, and I am also willing to deliver that, however a decision is needed on how to proceed.

[cowtowncoder]

Apologies for my silence here, but I really do not have enough expertise here to have specific opinion.
I can see benefits and challenges at high level.

What I think is needed is to figure out who should own this module, and decide. @beamerblvd wrote this module and would be the best candidate, but I think he has been busy lately.
I will send another note on the mailing list; but from my point, I think it is most important to have someone knowledgeable and active so that we can move forward. I don't want progress to stall due to fear of sometimes making sub-optimal decisions; mistakes can be fixed, implementations changed.

[zkiss]

I see two course of actions regarding the code:

• Merge this PR and add a new SerializationFeature to include timezone information
• Don't merge this PR, add the new SerializationFetaure.

Both ways will then require further dev work in this module and the joda module to handle the feature properly.

Merging this PR results in ISO compliant date strings but introduces loss of data and potentially a false sense of safety when people are dealing with ZonedDateTime objects - when they modify them they won't follow the timezone rules the original instance had before serialization.

[cowtowncoder]

@zkiss quick question: have I already asked for a CLA? (https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf)
I don't think so, apologies if I missed it.

Given that there has been very little feedback, I think I would go ahead and let you choose the path, with just one limitation: I think that the default value for new feature should be such that it does not break existing code. This means that ISO-8601 compatibility forcing should not be the default.

An additional suggestion I have, for path forward, would be to actually add code in Joda (etc) to detect additional timezone information, and simply discard it if it can not be handled. While I support strict handling in most cases, here it seems better to be bit more liberal with what you accept, because date/time handling is already a huge hassle. Adding more strictness seems counter-productive.

So:

1. Yes, for adding the new SerializationFeature (FORCE_ISO8601_COMPLIANT?)
2. Make default false, which in case of this module should mean "include timezone even at expense of ISO-8601 compatibility"
3. Retrofit applicable modules appropriately.

Does this make sense?

[cowtowncoder]

Forgot to mention: CLA should be printed, filled-in & signed and then emailed to info at fasterxml dot com.

[beamerblvd]

Please disregard this. I misunderstood in which file this change was taking place.

I don't understand why this change is necessary. According to the API documentation, Instant#toString already outputs a strictly ISO-compliant string.

[beamerblvd]

Given addressing of my comments, this looks good to me. But I want to better understand how this is supposed to address adding zone information—looks like that's missing.

[zkiss]

Thanks everyone for the feedback. Closing the PR because this would be a non-backwards compatible change - based on our discussion this should not be merged. Will implement the SerialisationFeature + handling in joda + jsr310. I am also planning on getting back here with more thoughts but just could not find the time yet.

[zkiss]

@cowtowncoder, @beamerblvd Regarding the way forward:

I fully understand and agree in general with the principle of introducing changes in a backwards compatible manner. However after giving it some thought, in my opinion it is impossible in this case to be backwards compatible. This is because the currently released joda module and jsr310 module have exactly the opposite behaviours.

Joda module currently does not preserve time zone information - it works the same way this module would with this PR merged. My guess is that joda module is used much more widely than this one.

For this reason I believe that the default value of the 'time zone preserving' config switch should be to not preserve the time zone information and produce iso8601 compliant strings.

If we agree in this, then my suggestion would be merging this PR and then adding the switch afterwards. If we don't agree, then the introduction of the new SerializationFeature will break backwards compatibility in the joda module, as it will start printing timezone information if someone upgrades to the version which contains the feature (This might not break server-to-server communication where both servers upgrade as the module would handle the presence of the unwanted time zone data, but old modules will fail because of the new format).

It is also a possibility that the joda module would ignore the new SerializationFeature, however I would not like that - I'd very much like the joda module and this one to be compatible with each other and behave the same way with the same ObjectMapper configuration.

Regarding the name of the SerializationFeature, I think it should be explicit about the fact that it is preserving timezone information. Something like WRITE_DATES_WITH_TIMEZONE or PRESERVE_ZONE. In my opinion being compliant with iso8601 by default should not be a question, as that is what the library has has been so good at so far.

[cowtowncoder]

Ok. Yes, I see the conundrum here. I also agree in that changing behavior of Joda module in backwards incompatible way would not be well received.

[cowtowncoder]

I concur with comments so far: I do not like backwards-incompatibility, and if a choice is needed, I would rather upgrade this module first, to allow transition, and postpone decision on Joda (if need be) until 2.7.

But as one minor starting point, since suggestion to add SerializationFeature.WRITE_DATES_WITH_ZONE_ID seems non-controversial, I added issue for adding it:

FasterXML/jackson-databind#794

and can easily add it. From discussions, it seems we could choose default either way; however, it sounds like preferred value would be false (in absence of compatibility constraints), so that the base behavior would be to start with ISO-8601 compatibility. If that is the eventual state we would want to get to, I would want to start with it. Otherwise we would have to change that default setting at a later point.

At this point it may be relevant to check out usage. From Maven Central, actual download counts for top 3 datatype modules are as follows:

• Joda: 90,590
• Guava: 57,586
• JSR-310: 46,309

so while Joda is the top dog, JSR-310 module is perhaps surprisingly almost 50% as popular, by mere download counts. This does not necessarily mean similar range of usage, as some frameworks may default to including many modules (Spring Boot and DropWizard may both include both, for example).
But it does suggest that there is non-trivial usage of this module.

[zkiss]

Sounds like we need to provide backwards compatibility options then.

My preferred solution for that would be writing a new serializer ZonedDateTimeWithZoneIdSerializer (marked as deprecated? But definitely explained in the javadocs that it is planned to be removed). The module would stop serializing zone ids by default. Users who need the old behaviour would have to do something like this:

ObjectMapper om = ... // get reference to OM
JSR310Module m = new JSR310Module();
m.addSerializer(ZonedDateTime.class, ZonedDateTimeWithZoneIdSerializer.INSTANCE);
om.registerModule(m);

Is this acceptable to everyone?

[cowtowncoder]

@zkiss Unfortunately I am not sure that is any better than other options -- many users simply upgrade versions of modules with no other changes, and expect things to work.
I think the worst kind of problem is one where compilation does not fail, and startup does not fail. but somewhere down the line something is different. This is what would happen here.

To me there are 2 main goals:

1. Allow use of ISO-8601 compliant, standard-configurable mechanism
2. Do not break simple-version-increase upgrades

The non-goal, then, would be "switch by default to new ISO-8601-compliant behavior", because I do not see a way to achieve that with (2).

But to simply achieve (1) and (2), how about following steps instead

I. Add new constructor, public JSR310Module(boolean useStdTimeZoneConfig); true will use new better mechanism, false 2.5-and-before defaults
II. Make no-arg constructor call it with false
III. (optional) Mark no-arg constructor as deprecated, to try to nudge users to use the explicit variant

This way default behavior is the old one (for drop-in-place upgrade), but users can easily switch to new, better default.

While not perfect, this seems to me a possible way forward. And at this point I would rather take that, than to wait for the perfect answer.

[cowtowncoder]

And one more. There is also one variant of the idea above: instead of adding new constructor, we could instead create a new module class, so that:

• Existing JSR310Module would default to existing behavior, but marked as deprecated
• New module, name to be determined (NewJSR310Module? ;-) ), would use the new configuration

we could easily refactor things so that most code would be shared.

This might actually be the better way to do this, mostly because creating instances using no-args constructors is bit easier with some frameworks. Plus we wouldn't need to consider whether to change no-args constructor of the existing module: we would instead deprecate and (eventually) remove that module class.

[tommack]

I think I like the new module idea. How aboutJava8DateModule or CompatibleJSR310Module.

[zkiss]

OK, let's go with the new module idea! I'd mark JSR310Module as @Deprecated and create a new module which serializes in ISO 8601 compatible way - and pretty soon honours the new SerializationFeature as well. As for the name, how about JavaTimeModule (making a reference to the package name)?

[emilyselwood]

Ignore earlier version of this comment. Page didn't load some comments so I missed half the conversation.

With a new module what will happen with the ObjectMapper.findAndRegisterModules() call? Will we get both modules?

[aisven]

+1 for new Module, deprecating the old Module. This makes things very clear to framework users.

[cowtowncoder]

@wselwood findAndRegisterModules() is an interesting part. If I remember correctly, it uses this file:

src/main/resources/META-INF/services/com.fasterxml.jackson.databind.Module

and we thereby control what is visible. However, our choice is pretty much just including one, both or none there. This is why I personally never use auto-detection; it tends to lead to accidental inclusion of things due to things getting added to classpath, often accidentally via transitive dependencies.

So the question here would be whether to include old or new module in classpath (I don't think "both" makes sense).
One possibility would be to include old one for 2.6, new for 2.7. But fundamentally it is just optimizing on point of breakage....

The nuclear option would be to create a new project. But that does seem... bit extreme.

[cowtowncoder]

As to name: I like both Java8DateModule and JavaTimeModule -- either one would work for me.

[emilyselwood]

By the sounds of it the best we can do in that case is optimize the point of breaking @cowtowncoder

My vote would go to JavaTimeModule but I am pretty happy with either name.

[cowtowncoder]

@wselwood Agreed. Either name is fine -- what is needed would be updates to PR, to allow both options.
Module name is trivial to change. I will be happy to merge version that does what we have discussed; a backwards-compatible version of existing module, and the new one that uses the new SerializationFeature.WRITE_DATES_WITH_ZONE_ID (which defaults to false).

And one more note on auto-registration, this:

FasterXML/jackson-jaxrs-providers#39

is a fix to JAX-RS provider, which now provides two sets of jars: one with auto-registration metadata; other without one. Maven classifier is used to specify one without metadata, to retain backwards-compatibility.
If it makes sense, we could do the same here if we want to.
But as with naming, this can be done after getting the base functionality in place.

[cowtowncoder]

@zkiss Do you think more information is needed here? Can I help in getting this implemented, as discussed? I am hoping to get this in 2.6 (ideally, 0-rc2).

[zkiss]

@cowtowncoder and all, thanks for all the input. I have pushed a new module class JavaTimeModule which should be doing what we discussed. I have duplicated the tests and explicitly copied the old ZonedDateTime test to check it is not broken.

The module is not autodetected by default. If there are any changes needed, please let me know!

[cowtowncoder]

@zkiss I trust that changes are good, but just in case others have comments, I'll wait until merging this. Hoping to merge tonight or tomorrow.

[zkiss]

One thing I forgot to mention: because of the duplication of all the tests (thinking in the short term here), this should either be merged real soon (before merging anything else with test changes) or a new way of testing the old and new behaviour should be implemented in this PR.

[cowtowncoder]

I'll postpone other merges, if any (none pending). Thank you for noting this problem. I am fine with duplication on short term.

[zkiss]

@cowtowncoder I think the old module JSR310Module should be deprecated but I could not make it deprecated because I get errors when doing mvn clean verify:

[WARNING] /home/zoltan/work/jackson-datatype-jsr310/src/test/java/com/fasterxml/jackson/datatype/jsr310/old/TestYearKeySerialization.java:[9,45] com.fasterxml.jackson.datatype.jsr310.JSR310Module in com.fasterxml.jackson.datatype.jsr310 has been deprecated
...
[ERROR] COMPILATION ERROR : 
[INFO] -------------------------------------------------------------
[ERROR] warnings found and -Werror specified
[INFO] 1 error

[cowtowncoder]

@zkiss I can deal with that one. Module sets up somewhat strict limits so that no warnings are allowed -- bit extreme, and perhaps could be reconsidered. But for now, I can just fix that, after merge.

[jontejj]

As this has been discussed quite much I think many of us are interested in a release. Any possibility to do it in the near future?

[cowtowncoder]

This will be included in 2.6.0-rc2, once I resolve couple of issues from jackson-databind (not related to this issue). Should be within a week.