Anchors are saved locale-sensitively and fail to parse on restart

[blubberdiblub]

Symptom

The anchors got saved as this (note the commas instead of periods):

anchors:
  farmdesert: world3:2205,98,70,00,5011,16:256,04:8,80
  farmswamp: world3:-5015,14,67,00,-3982,32:164,06:-0,15
  farmterracotta: world3:4345,02,76,00,7040,28:183,78:13,61

Which caused them to fail loading:

[00:12:44] [Server thread/WARN]: [Multiverse-Core] The location for anchor 'farmdesert' is INVALID.
[00:12:44] [Server thread/WARN]: [Multiverse-Core] The location for anchor 'farmswamp' is INVALID.
[00:12:44] [Server thread/WARN]: [Multiverse-Core] The location for anchor 'farmterracotta' is INVALID.

Problem description

Part of my locale settings (most notably LC_CTYPE, which triggers the problem, but not LC_NUMERIC) are on a German locale, i.e. de_DE.UTF-8.

This causes java to format numbers with a comma as decimal separator by default when using %.2f.

The thing is Multiverse shouldn't be using locale-sensitive formatting when trying to save a machine-readable representation of locations (happens for anchors only, as far as I can tell).

Relevant code lines

The problematic formatting happens here:

Multiverse-Core/src/main/java/com/onarandombox/MultiverseCore/utils/SimpleLocationManipulation.java

Line 55 in b4b0940

return String.format("%s:%.2f,%.2f,%.2f:%.2f:%.2f", location.getWorld().getName(),

And it gets called from this AnchorManager code (which is relevant for this issue):

Multiverse-Core/src/main/java/com/onarandombox/MultiverseCore/utils/AnchorManager.java

Line 115 in b4b0940

this.anchorConfig.set("anchors." + anchor, plugin.getLocationManipulation().locationToString(l));

It also gets called from those 2 locations, but they're not relevant for parsing, as the result is just logged:

Multiverse-Core/src/main/java/com/onarandombox/MultiverseCore/MVWorld.java

Line 442 in b4b0940

this.getName(), plugin.getLocationManipulation().locationToString(newSpawn));

Multiverse-Core/src/main/java/com/onarandombox/MultiverseCore/MVWorld.java

Line 450 in b4b0940

this.getName(), plugin.getLocationManipulation().locationToString(newerSpawn));

Deprecated remnants that don't have the problem

The funny thing is, there's still a deprecated LocationManipulation class lying around in the source code that isn't used anymore (as far as I can tell), but which does it correctly, by prescribing an English locale:

Multiverse-Core/src/main/java/com/onarandombox/MultiverseCore/utils/LocationManipulation.java

Line 67 in b4b0940

return String.format(Locale.ENGLISH, "%s:%.2f,%.2f,%.2f:%.2f:%.2f", location.getWorld().getName(),

Who is affected and when

Also note that this problem doesn't just affect German locales. There are more languages that don't use periods as decimal separator.

And it's not just LC_CTYPE that can trigger the problem (it shouldn't have in the first place, Java itself is to blame for using LC_CTYPE instead of LC_NUMERIC for number formatting, but that's beside the point). That's just my scenario, as I like to keep most other locale aspects on US English to avoid problems (well, it didn't in this instance).

A user that didn't set LC_CTYPE (which I wager are the majority), can trigger the problem by setting LC_ALL or LANG (when LC_ALL is empty) to a locale that doesn't use a period as decimal separator.

How to reproduce

I'm not sure whether the locale definitions (such as /usr/share/i18n/locales/de_DE) need to be installed, as I suspect Java uses its own.

1. Just start the server with env variables set appropriately (I'm using PaperMC here, but should be analogous for Spigot or Bukkit):

   LC_CTYPE=de_DE.UTF-8 java -Xms10G -Xmx10G -jar paperclip.jar
   

   Or

   LC_CTYPE="" LC_ALL=de_DE.UTF-8 java -Xms10G -Xmx10G -jar paperclip.jar
   

   Or

   LC_CTYPE="" LC_ALL="" LANG=de_DE.UTF-8 java -Xms10G -Xmx10G -jar paperclip.jar
   

2. Create some anchors with /mv anchor foobar

3. Observe that there are commas in anchors.yml instead of the expected decimal points

4. Restart the server (I suspect /mvreload might trigger it, too)

5. Observe that there are warnings for invalid locations of the saved anchors

6. Observe that the anchors are not present when trying to use them (/mvtp a:foobar)

Reproducing with jshell

It's also quite simple to pinpoint the behavior with jshell:

LC_CTYPE=de_DE.UTF-8 jshell

jshell> String.format("%.2f", 12.3456)
$1 ==> "12,35"

Using an explicit locale in the code fixes the problem regardless of external locale settings:

LC_CTYPE=de_DE.UTF-8 jshell

jshell> String.format(Locale.ENGLISH, "%.2f", 12.3456)
$1 ==> "12.35"

Suggested fix

I would like to see one of two things to happen for the problem to get fixed:

1. Either use Locale.ENGLISH in the String.format() call in the locationToString() function of the SimpleLocationManipulation class (the first link above) the same way as the deprecated code does (this would tie the log output of the spawn thingy linked above to coordinates with periods as decimal separator - I don't think anyone would complain)

2. Or have saveAnchorLocation() in AnchorManager use their own variant of location formatting (leaving the one instance of logging with a localized decimal separator).