Locale "" is deserialised as null if ACCEPT_EMPTY_STRING_AS_NULL_OBJECT is enabled

[pkraeutli]

Describe the bug

When trying to deserialise an empty JSON string as java.util.Locale, the resulting value is NULL, when ACCEPT_EMPTY_STRING_AS_NULL_OBJECT is set to true.
My expectation was that the empty string would be converted to Locale.ROOT.

Version information
2.13.5

To Reproduce

The following test fails:

class JsonDeserializationTest
{
    @Test
    void testDeserializeRootLocale() throws JsonProcessingException
    {
        ObjectMapper objectMapper = new ObjectMapper();
        objectMapper.configure(DeserializationFeature.ACCEPT_EMPTY_STRING_AS_NULL_OBJECT, true);

        assertEquals(Locale.ROOT, objectMapper.readValue("\"\"", Locale.class));
    }
}

When looking at the current source code at https://github.com/FasterXML/jackson-databind/blob/2.16/src/main/java/com/fasterxml/jackson/databind/cfg/CoercionConfigs.java#L241
It looks like CoercionAction.TryConvert should be returned in this case.

[pjfanning]

ACCEPT_EMPTY_STRING_AS_NULL_OBJECT means you want null if the string is empty. I can't understand your interpretation.

jackson-databind is highly configurable. Register your own Locale Deserializer if you don't like the default behaviour.

[pkraeutli]

The documentation of ACCEPT_EMPTY_STRING_AS_NULL_OBJECT suggests that it is meant for
"POJOs and other structured values ({@link java.util.Map}s, {@link java.util.Collection}s)".
https://github.com/FasterXML/jackson-databind/blob/2.16/src/main/java/com/fasterxml/jackson/databind/DeserializationFeature.java#L363

I tried writing a custom deserialiser but this does not work. The deserialiser is never picked up because the empty string is converted to NULL before the deserialiser has a chance to do anything. Also, the standard Locale deserialiser does exactly what I'd expect: https://github.com/FasterXML/jackson-databind/blob/2.16/src/main/java/com/fasterxml/jackson/databind/deser/std/FromStringDeserializer.java#L382

See also #1123

[JooHyukKim]

Okay, so what you are saying is both test below should pass right, @pkraeutli? So, it seems like deserializer (probably the FromStringDeserializer you mentioned) works only "after" the DeserializationFeature.ACCEPT_EMPTY_STRING_AS_NULL_OBJECT is applied.

   // passes, good
    public void testWhenDisabled() throws Exception {
        Locale disabled = JsonMapper.builder()
                .configure(DeserializationFeature.ACCEPT_EMPTY_STRING_AS_NULL_OBJECT, false)
                .build()
                .readValue("\"\"", Locale.class);
        assertEquals(Locale.ROOT, disabled);
    }
   
   // fails, not good!
    public void testWhenEnabled() throws Exception {
        Locale enabled = JsonMapper.builder()
                .configure(DeserializationFeature.ACCEPT_EMPTY_STRING_AS_NULL_OBJECT, true)
                .build()
                .readValue("\"\"", Locale.class);
        assertEquals(Locale.ROOT, enabled); // <--- fails, value of enabled is `null`
    }

[pkraeutli]

@JooHyukKim yes, exactly. As I understand it, ACCEPT_EMPTY_STRING_AS_NULL_OBJECT is there to deserialise invalid "" values as NULL (e.g. for Object, Maps, Lists, etc.).
In the case of Locale, the "" is a valid value so I think it should not be affected by ACCEPT_EMPTY_STRING_AS_NULL_OBJECT. Opinions may differ, of course :)

[pjfanning]

Even if @cowtowncoder agrees to make a change, it will likely not appear till 2.16.0 release which is many months away.

[JooHyukKim]

In the case of Locale, the "" is a valid value so I think it should not be affected by ACCEPT_EMPTY_STRING_AS_NULL_OBJECT, Opinions may differ, of course :)

In JSON specification perspective, it does not know about empty string being Local.ROOT, so seems like there is no straightforward solution here, but ideas. 🤔🙂

Even if @cowtowncoder agrees to make a change, it will likely not appear till 2.16.0 release which is many months away.

True, true.

[pkraeutli]

Sure, whatever you decide :) Either the current behaviour is as intended and then that's it, or it is a bug and then it may be fixed at one point.

For my part I will try to find another way to fix my particular issue in the meantime.

[JooHyukKim]

@pkraeutli Right, meanwhile, I tried to come up with some solution you might be interested in. To make sure, please refer to the documentation tho 👍🏻👍🏻

   class CustomLocaleDeserializer extends StdDeserializer<Locale> {

        public CustomLocaleDeserializer() {
            super(Locale.class);
        }

        @Override
        public Locale deserialize(JsonParser p, DeserializationContext ctxt) throws IOException {
            String text = p.getText();
            if (text != null && text.isEmpty()) {
                return Locale.ROOT;
            } else {
                return Locale.forLanguageTag(text);
            }
        }
    }

   @Test
    public void testDeserializeRootLocale() throws Exception {
        SimpleModule module = new SimpleModule();
        module.addDeserializer(Locale.class, new CustomLocaleDeserializer());
        ObjectMapper objectMapper = JsonMapper.builder()
                .enable(DeserializationFeature.ACCEPT_EMPTY_STRING_AS_NULL_OBJECT)
                .addModule(module)
                .build();
        objectMapper.registerModule(module);

        assertEquals(Locale.ROOT, objectMapper.readValue("\"\"", Locale.class));
    }

[pkraeutli]

Thanks @JooHyukKim ! I also came up with a solution that worked for me:

class JsonTest
{
    @Test
    void deserializeEmptyLocale()
    {
        ObjectMapper objectMapper = new ObjectMapper();
        objectMapper.enable(DeserializationFeature.ACCEPT_EMPTY_STRING_AS_NULL_OBJECT);
        objectMapper.coercionConfigFor(Locale.class)
            .setCoercion(CoercionInputShape.EmptyString, CoercionAction.TryConvert);

        assertNull(objectMapper.readValue("null", Locale.class));
        assertEquals(Locale.ROOT, objectMapper.readValue("\"\"", Locale.class));
    }
}

[cowtowncoder]

(removed my earlier comment which was based on misreading the issue)

So, yeah... Hmmh. I think @pkraeutli 's interpretations are quite close to what I'd expect. ACCEPT_EMPTY_STRING_AS_NULL_OBJECT is indeed more concerned about coercing otherwise invalid empty String into null.
And I think I would expect something like Locale.ROOT instead.

The only/main concern really is backwards-compatibility.
If anyone has time, it'd be good to see closed issues with "Locale" to see if there's some prior art to changes in this area; I have a feeling this has come up before.

So, let me think about this a bit.

[cowtowncoder]

Note: looks as if with #1123 handling was changed to get from "" to Locale.ROOT.

I think we should ignore ACCEPT_EMPTY_STRING_AS_NULL_OBJECT for Locale for 2.16.

[cowtowncoder]

If anyone wants to do PR, I'd be happy to review. Or maybe I'll find time to try it out myself, now that we have a test via #4012.

[JooHyukKim]

If anyone wants to do PR, I'd be happy to review. Or maybe I'll find time to try it out myself, now that we have a test via #4012.

Would the PR best proceed by implementing new com.fasterxml.jackson.databind.deser.std.LocaleDeserializer?

nvm. let's save your time by looking into it myself.

[cowtowncoder]

Right, I think it might not be necessary to register new implementation but modify shared FromStringDeserializer or whatever it was.

[pkraeutli]

When debugging, the part here https://github.com/FasterXML/jackson-databind/blob/2.16/src/main/java/com/fasterxml/jackson/databind/cfg/CoercionConfigs.java#L237 caused the conversion from String to Locale to be skipped.

Maybe a change to this block is sufficient so that it returns CoercionAction.TryConvert instead of CoercionAction.AsNull it the target type is Locale?

[cowtowncoder]

@pkraeutli Yes, after thinking it through I think you are right; moving

if (targetType == LogicalType.OtherScalar) { ... }

before the other check would fix this.

[cowtowncoder]

Implemented as suggested, will be in 2.16.0. Added a note wrt behavioral change on https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.16.

[pkraeutli]

@cowtowncoder @pjfanning @JooHyukKim thanks a lot! Much appreciated.