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

The LocalDateTime(Object instant, DateTimeZone zone) constructor ignores the timezone #454

Closed
veverone opened this issue Nov 24, 2017 · 9 comments

Comments

@veverone
Copy link

veverone commented Nov 24, 2017

Key information

  • Joda-Time version:
    2.9.3

  • Result of TimeZone.getDefault()
    sun.util.calendar.ZoneInfo[id="Europe/Athens",offset=7200000,dstSavings=3600000,useDaylight=true,transitions=138,lastRule=java.util.SimpleTimeZone[id=Europe/Athens,offset=7200000,dstSavings=3600000,useDaylight=true,startYear=0,startMode=2,startMonth=2,startDay=-1,startDayOfWeek=1,startTime=3600000,startTimeMode=2,endMode=2,endMonth=9,endDay=-1,endDayOfWeek=1,endTime=3600000,endTimeMode=2]]

  • Result of DateTimeZone.getDefault()
    Europe/Athens

Problem description

The LocalDateTime(Object instant, DateTimeZone zone) constructor ignores the timezone due to iChronology = chronology.withUTC();
The somewhat analogue LocalDateTime(long instant, DateTimeZone zone) gives the answer considering the TimeZone.

Test case

new LocalDateTime(new LocalDateTime(2012, 1, 1, 0, 0), DateTimeZone.getDefault())
returns 2012-01-01T00:00:00.000

while....

new LocalDateTime(new LocalDateTime(2012, 1, 1, 0, 0).getLocalMillis(), DateTimeZone.getDefault())
returns 2012-01-01T02:00:00.000

@jodastephen
Copy link
Member

The final piece of code is incorrect. It uses getLocalMillis() which returns the local milliseconds from 1970-01-01T00:00. You are then passing that value in where the **instant"" milliseconds is expected. This is why local milliseconds should not normally be used.

By passing local in where instant is expected, you implicitly convert the local millis to instant millis based on UTC, whereas you need to pass in the correct instant millis for Athens, which is 2 hours different. eg.

var localMillis = new LocalDateTime(2012, 1, 1, 0, 0).getLocalMillis();
var instantMillis = zone.convertLocalToUTC(localMillis, false);
var result = new LocalDateTime(instantMillis, zone);

@veverone
Copy link
Author

veverone commented Nov 24, 2017 via email

@jodastephen
Copy link
Member

No, there is no bug. In order to perform calculations on local millis, the code uses the UTC version of the chronology. Its an internal detail, and correct.

@veverone
Copy link
Author

veverone commented Nov 24, 2017

Thank you very much for your quick answers.
Please, allow me one more question. I believe it will help me.

Why do I get the same result while printing, to the console, the objects created below?

new LocalDateTime(new LocalDateTime(2012, 1, 1, 0, 0), DateTimeZone.getDefault())
2012-01-01T00:00:00.000
new LocalDateTime(new LocalDateTime(2012, 1, 1, 0, 0), (DateTimeZone) null))
2012-01-01T00:00:00.000
new LocalDateTime(new LocalDateTime(2012, 1, 1, 0, 0), DateTimeZone.forOffsetHours(8))
2012-01-01T00:00:00.000

Kind regards,
Bogdan C.

@veverone
Copy link
Author

What are your thoughts about this?

@veverone
Copy link
Author

veverone commented Nov 29, 2017

Hi,

Would you, please, look into my last question / test case?

Thank you very much

@veverone
Copy link
Author

veverone commented Dec 3, 2017

Any chance you can post your opinion about the last use case?

@jodastephen
Copy link
Member

I see the problem - you expect the new LocalDateTime to be affected by the time-zone - but I'm not sure its practical to do anything about it (other than to document the behaviour better). While with LocalDateTime it might well be possible to use the time-zone and convert the local time in accordance with it, doing so for the same constructor on LocalDate or LocalTime would be confusing. In addition, since this constructor has worked this way for over 15 years, changing the behaviour now isn't really viable. As such, the only way to achieve your goal would be to add a new factory method.

This can also be worked around, along the lines of base.toDateTime(DateTimeZone.UTC).withZone(DateTimeZone.forOffsetHours(8)).toLocalDateTime().

Its also important to bear in mind that Joda-Time has been replaced by java.time.* in Java 8, so limited efforts now go into Joda-Time. Note that java.time does not have the LocalDateTime time-zone conversion you are trying to do either - the general approach is to use DateTime in Joda-Time when using time-zones.

@veverone
Copy link
Author

veverone commented May 8, 2018

Huh, I thought you would never answer. But you did and you did it in a diplomatic way.
Thank you for that!

I understand and I agree with you that, at this point, you can't do anything about it.
Given this, your suggestions here are welcomed! I'll keep them in mind.

Have a great day forward!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants