-
Notifications
You must be signed in to change notification settings - Fork 82
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
JodaModule creates org.joda.time.DateTime that fails .equals when compared with original #50
Comments
After looking into this, I don't think this is a bug but expected behavior. The underlying timestamp is equal, but the timezone differs. This because test is constructing If identical representation is desired, one way to do this is to use |
I just ran into this. With all of the work to get WRITE_DATES_WITH_ZONE_ID working, I was surprised to see this basic thing so broken. The whole point of a DateTime is that it is both a moment in time and a timezone. If WRITE_DATES_AS_TIMESTAMPS is off, this makes no difference. But when WRITE_DATES_AS_TIMESTAMPS is on, changing the timezone in the ISO 8601 string is just wrong. This is particularly apparent when using WRITE_DATES_WITH_ZONE_ID. Now we have Jackson outputting strings like "2015-09-12T01:40:09.707Z[America/Chicago]" which makes no sense at all. The jackson-datatype-jsr310 library gets this right and will output "2015-09-11T20:42:52.295-05:00[America/Chicago]" for a ZonedDateTime (rough equivalent to Joda's DateTime). |
As usual, pull requests welcome. Even if just for failing test case. I am going over accumulated bugs remaining in 2 .6.1 so this could be fixed for 2.6.2. |
Hi, |
I just noticed that your test is wrong. The problem resides in the creation of the DateTime you are using for the test. See bellow the right test.
However, this works only because your test data is in UTC. Here is a failing one that illustrates your problem more properly :
What I noticed is that the issue is in the Deserialisation in this case. Jackson set the created DateTime in UTC (there is no reason not to set the right TZ in the DateTime, the question seemed to be an issue regarding a comment in the code of DateTimeDeserializer
I don't agree with Tatu, one will expect Jackson to read the full value of the information fed to it. TZ is an important information that we want to keep. |
…he logical behavior of the (De)Ser
I didn't fix the unit tests because that means changing the default behavior of that plugin. Hope we can discuss it.
where MyDateTimeXXXX are subclasses of the original ones and override the (de)serialize method with my fix. |
@wattazoum Please bring these up on mailing lists. I am not an expert on date/time handling, so you better convince those members who are, like @tommack @beamerblvd. As to keeping TZ, handling, there have been extensive discussions back and forth, including cross-module behavior (core databind, Java 8 date/time, Joda) and settings. There are no easy fixes, and changes that affect backwards compatibility are tricky. At this point there is not much value in Jackson 1.x -> 2.x compatibility, however, compared to compatibility between 2.x minor versions. |
I didn't find the mailing list but I used the forum to start the discussion |
@wattazoum Appreciate your effort, but unfortunately I do not think many of developers participate in that forum (I do try to follow up myself). Mailing group is at: |
@wattazoum thank you for sending the question on the dev mailing list. This would be good time to get things started for changes (if any) for 2.8. With 2.7 patch releases we can fix bugs, but need to be cautious on otherwise changing behavior (definition of bug/flaw, and other sub-optimal behaviors may be arbitrary of course but still). |
We've run into a similar, if not the same, problem when attempting to use the latest version (2.8.6) with our application. Our application makes extensive use of Unfortunately, Our attempt to fix this is: JodaModule jodaModule = new JodaModule();
JacksonJodaDateFormat jacksonJodaDateFormat = new JacksonJodaDateFormat(FormatConfig.DEFAULT_DATETIME_PARSER, TimeZone.getDefault());
DateTimeDeserializer deserializer = new DateTimeDeserializer(DateTime.class, jacksonJodaDateFormat);
jodaModule.addDeserializer(DateTime.class, deserializer);
ObjectMapper objectMapper = new ObjectMapper();
objectMapper.registerModule(jodaModule); Unfortunately, the
This is because the declaration of DateTimeDeserializer is: public class DateTimeDeserializer extends JodaDateDeserializerBase<ReadableInstant> Shouldn't this be: public class DateTimeDeserializer extends JodaDateDeserializerBase<DateTime> We made this change in a private copy of Is this safe / correct, or are we missing something? Thanks in advance for your attention to this. Jim Renkel |
I think you are right. Looks like a bug. |
Will this be fixed? Or should we submit our fix? We use a lot of open source code, but I don't think we've yet contributed to any. This may be a good opportunity for us to start doing that, although we will prolly need some coaching. Do you know of a good, step-by-step tutorial on how to do this? Thanks, Jim |
No changes should be needed; although type assignment may not be optimal there is a better way to instantiate deserializer, used by addDeserializer(DateTime.class,
DateTimeDeserializer.forType(DateTime.class));
addDeserializer(ReadableDateTime.class,
DateTimeDeserializer.forType(ReadableDateTime.class));
addDeserializer(ReadableInstant.class,
DateTimeDeserializer.forType(ReadableInstant.class)); |
Originally this was FasterXML/jackson-databind#617
I was surprised to find that a org.joda.time.DateTime instance was not .equals to the org.joda.time.DateTime that produced the JSON in the first place.
There is a git repository supported by a gradle build that demonstrates the issue at https://github.com/robertkuhar/jacksonjodawtf
The text was updated successfully, but these errors were encountered: