In CollectionDeserializer, JsonSetter.contentNulls is sometimes ignored

[k163377]

Search before asking

• I searched in the issues and found nothing similar.

Describe the bug

There seems to be a problem with the following code

jackson-databind/src/main/java/com/fasterxml/jackson/databind/deser/std/CollectionDeserializer.java

Lines 409 to 423 in 896c1cf

	if (p.hasToken(JsonToken.VALUE_NULL)) {
	// 03-Feb-2017, tatu: Hmmh. I wonder... let's try skipping here, too
	if (_skipNullValues) {
	return result;
	}
	value = _nullProvider.getNullValue(ctxt);
	} else if (typeDeser == null) {
	value = valueDes.deserialize(p, ctxt);
	} else {
	value = valueDes.deserializeWithType(p, ctxt, typeDeser);
	}
	if (value == null) {
	_tryToAddNull(p, ctxt, result);
	return result;
	}

Jackson may deserialize empty characters in an array on JSON to null.
In this case, JsonToken will be VALUE_STRING instead of VALUE_NULL.

On the other hand, in the code presented, Nulls.SKIP and Nulls.FAIL are only applied if it is VALUE_NULL.
(To be precise, Nulls.SKIP works. However, it seems that the decision regarding _skipNullValues is being made in two places, which I feel should be improved).

Version Information

2.19.0

Reproduction

import com.fasterxml.jackson.annotation.JsonSetter;
import com.fasterxml.jackson.annotation.Nulls;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.exc.InvalidNullException;
import org.junit.jupiter.api.Test;

import java.util.List;

import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;

public class TestGitHubXXX {
    static class Dst {
        private List<Integer> list;

        public List<Integer> getList() {
            return list;
        }

        public void setList(List<Integer> list) {
            this.list = list;
        }
    }

    @Test
    public void nullsFailTest() {
        ObjectMapper mapper = new ObjectMapper();
        mapper.configOverride(List.class).setSetterInfo(JsonSetter.Value.forContentNulls(Nulls.FAIL));

        assertThrows(
                InvalidNullException.class,
                () -> mapper.readValue("{\"list\":[\"\"]}", new TypeReference<Dst>(){})
        );
    }

    @Test
    public void nullsSkipTest() throws JsonProcessingException {
        ObjectMapper mapper = new ObjectMapper();
        mapper.configOverride(List.class).setSetterInfo(JsonSetter.Value.forContentNulls(Nulls.SKIP));

        Dst dst = mapper.readValue("{\"list\":[\"\"]}", new TypeReference<>() {});

        assertTrue(dst.getList().isEmpty());
    }
}

Expected behavior

The JsonSetter.contentNulls must function properly.

Additional context

This was discovered by a bug report to kotlin-module.
FasterXML/jackson-module-kotlin#976

[cowtowncoder]

Hmmmh. I am not quite sure, this is unfortunately... not clear cut.

The problem is actually with coercion of empty String into null. In general, Nulls.xxx should only need to be checked for input.

But I can see how it's problem for end users. Big problem tho is that now we have tons of places where check needs to be done twice: first for input nulls, and second for JsonDeserializer coerced nulls too.

At the same time, although ideally coercion would not be done in cases where nulls are prohibited, coercion handlers do not have the context to determine this.

So perhaps it is necessary to apply _nullProvider for these two different cases.

[cowtowncoder]

Fixed for inclusion 2.19.1 (and 3.0.0)