Read an empty Locale string back as Locale.ROOT#3045
Conversation
Gson serializes Locale.ROOT to an empty string, but reading it back threw a NullPointerException: with no tokens the language stayed null and was passed to new Locale(null). Default the language to the empty string so the empty form round-trips to Locale.ROOT.
Marcono1234
left a comment
Marcono1234
left a comment
There was a problem hiding this comment.
Good catch!
I have added a few suggestion comments, hopefully they are useful. The direct project members might have additional comments.
| String locale = in.nextString(); | ||
| StringTokenizer tokenizer = new StringTokenizer(locale, "_"); | ||
| String language = null; | ||
| String language = ""; |
There was a problem hiding this comment.
This change seems to work (new Locale("") creates something which acts like Locale.ROOT), but maybe it would be better to explicitly handle this case.
That is, keep = null here and then below add a if (language == null) { return Locale.ROOT; }?
What do you think?
There was a problem hiding this comment.
Not directly related to these changes, but maybe could omit the variant == null here?
- if (country == null && variant == null) {
+ if (country == null) {If I understand the code above correctly, due to the hasMoreElements() checks country == null implies variant == null. The current (redundant?) variant == null check irritates null analysis (e.g. in IntelliJ IDEA), making it think for new Locale(language, country, variant) country can actually be null.
(Side note: IntelliJ did also identify the new Locale(language) as potentially causing a NullPointerException, which you proved in your PR here.)
This is only a suggestion though; feel free to ignore this and keep the code as it is.
|
|
||
| @Test | ||
| public void testLocaleDeserializationRoot() { | ||
| // Gson serializes Locale.ROOT to "", so it must read "" back to Locale.ROOT. |
There was a problem hiding this comment.
For completeness it would probably be good to have also a serialization test which shows that Locale.ROOT is actually serialized as "\"\"".
Per review: default language to null and return Locale.ROOT explicitly for an empty tag, drop the redundant variant == null check (country == null already implies it), and add a test that Locale.ROOT serializes to an empty string.
|
Thanks @Marcono1234! Adopted all three:
(And credit to IntelliJ for flagging the |
2 participants
Gson serializes
Locale.ROOTto an empty string (Locale.toString()returns""), but deserializing""back to aLocalethrew aNullPointerException: the tokenizer produced no tokens, solanguagestayednulland was passed tonew Locale(null).Defaulting
languageto the empty string makes the empty form round-trip toLocale.ROOT. Added a regression test.AI assistance disclosure
This contribution was produced with the help of an AI pipeline. The pipeline processed a large amount of source code to surface suspected bugs, reproduced a subset of them with failing unit tests and generated candidate fixes, and prepared pull requests from the ones that held up. Each PR was then reviewed and verified by a human before being opened: the fix and test were checked by hand and the test was confirmed to fail before the change and pass after.