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

joda-time returns the new renamed time zone if DateTimeZone.forID is called with the old time zone id #524

Open
TomWolk opened this issue Feb 20, 2020 · 17 comments
Labels

Comments

@TomWolk
Copy link

TomWolk commented Feb 20, 2020

For renamed time zones org.joda.time.DateTimeZone.forID(String id) returns a time zone object that uses the new id even if you use the old id as the method parameter.

This brings me into trouble when I let two servers that have differen joda versions talk to each other. For instance a server with joda-time 2.3 sends time zone Asia/Rangoon to a server with joda time 2.10.5. The latter will change the time zone to Asia/Yangon when .DateTimeZone.forID is called in the deserialization. When the servers sends the time zone back to the former server, joda-time 2.3 does not know of the time zone Asia/Yangon that was introduced in 2017

By the way this change of names is not the case for java.util.TimeZone.getTimeZone(String ID) and java.time.ZoneId.of(String zoneId).

  • Joda-Time version 2.10.5

Test testRangoonDateTimeZone fails for 2.10.5 but should pass

public void testRangoonDateTimeZone() throws Exception {
    DateTimeZone zoneOld = DateTimeZone.forID("Asia/Rangoon");
    assertEquals(zoneOld.getID(), "Asia/Rangoon");

    DateTimeZone zoneNew = DateTimeZone.forID("Asia/Yangon");
    assertEquals(zoneNew.getID(), "Asia/Yangon");
}

public void testRangoonJavaUtilTimeZone() throws Exception {
    TimeZone zone = TimeZone.getTimeZone("Asia/Rangoon");
    assertEquals(zone.getID(), "Asia/Rangoon");

    TimeZone zoneNew = TimeZone.getTimeZone("Asia/Yangon");
    assertEquals(zoneNew.getID(), "Asia/Yangon");
}

public void testRangoonJavaTimeZoneId() throws Exception {
    ZoneId zone = ZoneId.of("Asia/Rangoon");
    assertEquals(zone.getId(), "Asia/Rangoon");

    ZoneId zoneNew = ZoneId.of("Asia/Yangon");
    assertEquals(zoneNew.getId(), "Asia/Yangon");
}
TomWolk pushed a commit to TomWolk/joda-time that referenced this issue Feb 20, 2020
TomWolk pushed a commit to TomWolk/joda-time that referenced this issue Feb 20, 2020
@TomWolk
Copy link
Author

TomWolk commented Feb 21, 2020

The Pull Request would also fix #427

@TomWolk
Copy link
Author

TomWolk commented Apr 28, 2020

@jodastephen , when can I expect this to be released?

@jodastephen
Copy link
Member

This is a tricky one. The code has worked this way for nearly 20 years, and normalizing deprecated IDs to their replacements could well be considered to be good behaviour.

If Joda-Time stopped normalizing, many applications would start seeing IDs they didn't see before, and that could break other systems that aren't expecting the older IDs.

My problem is that I have to weigh up whether the PR would cause more issues than it would fix. I have to take into account the 20 years of working this way, and the project being in maintenance mode. Given all this, I struggle to come to the conclusion that merging this would be wise.

@TomWolk
Copy link
Author

TomWolk commented May 1, 2020

I don't think applications will start seeing IDs they don't know about because the applications only get the old ID if the use the old name (which is practically the ID as a string) and if they do so, they are most likely able to handle it. Therefore I don't think any application or user will be surprised by the behaviour of DateTimeZone.forID returning the ID matching the name. I would suggest that that's what most people are expecting.

The new IDs are much more likely to break an application in a distributed environment because the old IDs are known to all systems but the new IDs are probably not known.

Additionally the java classes java.util.TimeZone and java.time.ZoneId work lik that (meaning not normalizing deprecated IDs) and I don't think that anyone is complaining about those classes.

I don't see how Joda-Time was working 20 years "one same predictable way". It's the opposite. After every update of the time zone data, Joda-Time normalizes (changing would be more accurate) a different set of IDs. Consequently assuming there are 3 time zone updates a year, you end up with 60 slighlty incompatible Joda-Time versions after 20 years. Of course this is an exaggeration compared to the actual situation because time zone ID changes are rare and system set ups where differen versions of Joda-Time are also rare. But the fact remains that there are a couple of Joda-Time versions whos DateTimeZone.forID methods are behaving differently. And not because of the logic in the method implementation but because of the data behind it.

I totally understand your desire to retain the behaviour of normalizing IDs because it is hard to rule it outthat someone out there relies on it. And in general it is a good practice in library developement to change as little as possible to not surprise anyone.

But I think that normalizing old Time Zone IDs to new ones was a bad design choice in the first place and the downsides of it are much greater than the benefits.

@broneill
Copy link
Contributor

Would it be possible to add a method named forExactID that does the new thing? The original forID method can remain the same for compatibility.

@TomWolk
Copy link
Author

TomWolk commented Dec 27, 2020

Yes, I think that would be possible, but you have to use forExactID in the method readResolve() to ensure that the TimeZone-ID does not change during deserialization.

@broneill
Copy link
Contributor

With serialization, there's an assumption that a proper time zone object was constructed in the first place. Normalization might have already occurred, and so making readResolve be more strict won't break compatibility. Calling forExactID from readResolve preserves the general contract of serialization and represents the original logical object as intended.

I can't imagine how this breaks compatibility, because even for normalization to work, both the new and old time zone ids must be known. If a time zone is serialized with an id unknown by the other side, this never works, regardless of the proposed changes. If for some reason an old id was removed from the rules without a replacement, serialization of the time zone fails regardless of where or if normalization takes place.

@TomWolk
Copy link
Author

TomWolk commented Dec 27, 2020

Well... I had a case where 3 servers had to communicate with serialization/deserialization happening between them. First time between server A and B, sencond time between server B and C. If the server B in the middle hast a newer version of JodaTime that normalizes a deprecated TimeZone-ID than the deserialization breaks on server C. Server B changes the the ID upon deserialization to a new ID that the Server C does not know and than the deserialization on server C is impossible.

From the outside it looks like a bug, because Server A sends a TimeZone-ID that Server C should understands. But because Server B silently normalizes it to a newer TimeZone-ID the whole communication breaks.

The exception upon deserialization would also occure if Server B sends back a response with the changes TimeZone-ID to Server A who doesn't know it.

@broneill
Copy link
Contributor

The set of time zones that can be supported among the three servers is defined as the intersection of all sets. The current serialization technique favors a union of the sets, which doesn't make any sense. No argument from me that serialization should avoid normalization and send the time zone in the fashion that's least likely to cause problems. Just don't change the forID behavior, because that can break compatibility even when serialization isn't used.

FYI: I wrote the original Joda-time time zone code, although I've not contributed to the project in ages. I just stumbled upon this issue out of curiosity.

TomWolk pushed a commit to TomWolk/joda-time that referenced this issue Oct 13, 2021
TomWolk pushed a commit to TomWolk/joda-time that referenced this issue Oct 13, 2021
TomWolk pushed a commit to TomWolk/joda-time that referenced this issue Oct 13, 2021
…e ids. DateTimeZone.forIDNoMapping() will return an object with an id exactly as provided
TomWolk pushed a commit to TomWolk/joda-time that referenced this issue Oct 13, 2021
…e ids. DateTimeZone.forExactID() will return an object with an id exactly as provided
@TomWolk
Copy link
Author

TomWolk commented Oct 13, 2021

I have changed the Pull Request. Now only readResolve in deserialization uses the method forExactID that does not normalize time zones.

@kluever
Copy link
Contributor

kluever commented Sep 8, 2022

I'm still collecting some data, but the recent Kiev -> Kyiv rename (2022b and onward) cause a few production outages at Google for folks still using JodaTime (since JodaTime immediately normalizes Kiev to Kyiv).

There will always be a problem when a client is updated to a new tzdb before the server, but it's localized to the few handful of "quick clients". However, if JodaTime is in the middle of the stack, and normalizes Kiev to Kyiv, then anything running an old tzdb that lives "down the stack" will have problems for all requests with either Kiev or Kyiv.

I'm wondering if this recent timezone rename has impacted other companies/users as well?

@hackmann
Copy link

We are hit by this Kiev/Kyiv too.

I expected this to work

    @Test
    void zoneIdEuropeKiev_v1() {
        final ZoneId europeKiev = ZoneId.of("Europe/Kiev");
        final DateTimeZone europeKyiv = DateTimeZone.forID("Europe/Kiev");
        final ZoneId roundtrip = ZoneId.of(europeKyiv.getID());
        assertEquals(europeKiev, roundtrip);
    }

But it fails with java.time.zone.ZoneRulesException: Unknown time-zone ID: Europe/Kyiv

Second attempt

    @Test
    void zoneIdEuropeKiev_v2() {
        final ZoneId europeKiev = ZoneId.of("Europe/Kiev");
        final DateTimeZone europeKyiv = DateTimeZone.forID("Europe/Kiev");
        final ZoneId roundtrip = europeKyiv.toTimeZone().toZoneId();
        assertEquals(europeKiev, roundtrip);
    }

This is actually worse as Europe/Kiev now silently becomes GMT instead of failing

I expected that Joda time's DateTimeZone.toTimeZone did a reserve conversion of what is done in DateTimeZone.forID

@tntim96
Copy link

tntim96 commented Nov 10, 2022

Also seeing this in release 2.12.1

  @Test
  public void testAustraliaCurrieDateTimeZone() {
    DateTimeZone zoneOld = DateTimeZone.forID("Australia/Currie");
    assertEquals("Australia/Currie", zoneOld.getID());
  }

...result is

org.junit.ComparisonFailure: 
Expected :Australia/Hobart
Actual   :Australia/Currie

@jodastephen
Copy link
Member

Proposed change in #676

This retains auto-conversion of the oldest pre-1993 country/fixed IDs but allows all other IDs of the modern style to be retained.

I don't believe it creates any new compatibility issues where different versions of Joda-Time are on different servers. It could break compatibility of anyone who expects zone IDs to be normalized, but I'm now willing to take that risk.

Please comment if you have or would like to test this before release.

@kluever
Copy link
Contributor

kluever commented Nov 24, 2022

Awesome --- I patched this and I'm running this against all tests at Google and will report back any issues.

EDIT: I'm immediately seeing a few tests fail with: The datetime zone id 'Africa/Accra' is not recognised

Oh hmm, it seems we have a locally patched copy of ZoneInfoCompiler (to read from our own tzdb files)...so just dumping the current ZoneInfoCompiler over our own patched copy is causing troubles. This might take some time to untangle (3-way merge and all...). I'll try to get back to this next week or so.

@jodastephen
Copy link
Member

@kluever Did you get anywhere with the testing? thanks

@kluever
Copy link
Contributor

kluever commented Dec 20, 2022

Sadly, no --- I haven't made any significant progress. Unfortunately our internal copy of ZoneInfoCompiler hasn't been updated in quite some time, which is complicating the testing.

@jodastephen jodastephen added the v3 label Jan 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

6 participants