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

Handling Nulls Inside Lists where List Element Type is @NotNull #2500

Closed
mikeholler opened this issue Oct 15, 2019 · 20 comments
Closed

Handling Nulls Inside Lists where List Element Type is @NotNull #2500

mikeholler opened this issue Oct 15, 2019 · 20 comments
Milestone

Comments

@mikeholler
Copy link

Hey there! I'm using jackson-module-kotlin and I like my null safety, but Jackson doesn't appear to respect these desires. I've been fighting with this issue for nearly 4 hours, and the guidance I've found is very confusing. I'm creating the issue here because I believe the solution lies with jackson-databind and not jackson-module-kotlin.

data class ObjectWithStringList(val strings: List<String>)

@Test
fun `list contains null`() {
    expectThrows<JsonProcessingException> {
        val result = ObjectMapper().registerKotlinModule().readValue<ObjectWithStringList>("""{ "strings": ["a", null, "b"] }""")
    }
}

ObjectWithStringList defines strings as List, which means that users should expect that the strings inside the list are never null (List<String?> would mean it could contain nulls). When I run this test, however, it fails (the exception is not thrown). What happens instead is result[1] evaluates to null, which should never happen according to Kotlin.

What do I want? Ideally I would want jackson-module-kotlin to respect collection element types' nullity, however I realize this is idealistic and may be difficult. Instead, I would like the default behavior of deserializing any list type with nulls (whether String or String?) to throw an exception. And then I would like some way to override this behavior if I really want to allow nulls in my list.

I've gone down the rabbit hole and tried a large number of things (which I can summarize upon request, but for brevity's sake, decided not to here), and have been unable to find anything acceptable. The best solution I've found so far is implementing my own collection type (see below) but the drawback to this solution is that I still want it to appear as List, not ArrayListOfNotNull.

data class ObjectWithStringList(val strings: ArrayListOfNotNull<String>)
class ArrayListOfNotNull<E: Any> : ArrayList<E> {
    constructor(): super()
    constructor(initialSize: Int): super(initialSize)
    constructor(elements: Collection<E>): super(elements) { require(elements.none { it == null }) }
    override fun addAll(elements: Collection<E>): Boolean { require(elements.none { it == null }); return super.addAll(elements) }
    override fun addAll(index: Int, elements: Collection<E>): Boolean { require(elements.none { it == null }); return super.addAll(index, elements) }
    override fun add(element: E): Boolean { requireNotNull(element); return super.add(element) }
    override fun add(index: Int, element: E) { requireNotNull(element); super.add(index, element) }
}

Also note that this did not work (I was really hoping it would):

data class ObjectWithStringList(@JsonDeserializer(`as` = ArrayListOfNotNull::class) val strings: List<String>)

Even if it did work, however, it's not the default behavior (which was my original preference).

With the plethora of ways to integrate into Jackson Databind, what is the recommended approach here? Additionally, I have to believe this affects lots of Kotlin users, whether they realize it or not (and I think the latter is more likely). As a result, I'd like to see some documentation added for handling this type of situation, because I could not find much of substance.

@cowtowncoder
Copy link
Member

This is probably more related Kotlin (although certainly would need help from databind), since core Jackson has absolutely no idea of ? decoration that Kotlin includes somehow. Kotlin module would need to determine this (I don't know what is used at JVM level, maybe annotation of some kind in bytecode).

But one big challenge is that although databind does have many mechanisms for skipping or ignoring null values for properties, same is not true for Lists. Big part of this is that in Lists, one really should not just automatically ignore anything: being index-based it is extremely confusing to just not see something for which input had value, or placeholder.

Having said that, Jackson 2.9 did add significantly to configurability of null handling, with annotations like:

@JsonSetter(nulls=Nulls.SKIP, contentNulls = Nulls.FAIL)

(see https://medium.com/@cowtowncoder/jackson-2-9-features-b2a19029e9ff under "Null replacement/error/skipping (on deserialization)")

which would seem promising, but. This annotation allows specifying handling for both values of properties ("main" nulls, aka "value nulls") and contents of values.
So in case of Lists, you would want to specify behavior for contentNulls to either SKIP (will just remove them, effectively) or FAIL (to throw exception indicating invalid input).

So: I think this might work for you.

I think Kotlin module does not change Collection handling a lot, so it might "just work". Unit tests for pure Java can be found from:

src/test/java/com/fasterxml/jackson/databind/deser/filter/NullConversionsForContentTest.java

@mikeholler
Copy link
Author

mikeholler commented Oct 15, 2019

Hey @cowtowncoder thank you for the response! I don't expect jackson-databind to understand ?, but I also know that jackson-module-kotlin has also discussed the understanding of nullity annotations in lists and come to the conclusion here (TLDR; won't happen): FasterXML/jackson-module-kotlin#27

It's for that reason that I'm writing you here to understand what we can do in Kotlin using just the jackson-databind libs, because it seems that's where our best chance at a solution lies. We're on 2.9.9 right now I think, and the @JsonSetter annotation was a good suggestion, but it also doesn't seem to work in Kotlin: FasterXML/jackson-module-kotlin#250

I know there are a lot of bindings available in Jackson for modifying functionality; are there any you see that would work somewhat similarly (via annotation, ideally, but open to other solutions) to what you described that Kotlin might support? It's these kinds of sharp edges I kept stumbling on while I was looking for a solution and I hoped that you or someone else here who knows Jackson deeply could help me find a good compromise.

@cowtowncoder
Copy link
Member

@mikeholler I don't think #250 is quite related here: TBH I am not sure I understand what user is trying to do there (and whether it should work or not), but it is not about List handling of nulls.
So I would not assume that this means @JsonSetter in general does not work.

But aside the question of making @JsonSetter work, possible next step would be to make Kotlin module indicate that by default nulls within Collections should be SKIPped. This would be possible via AnnotationIntrospector since databind itself is determining existing of @JsonSetter using its default JacksonAnnotationIntrospector; so whatever configured AI tells it is the truth.

Now, back to ?: what I still do not quite know is exactly how Kotlin uses that information. I guess it is possible it might only be used similar to Java generics, i.e. compile-time only info. If so, we couldn't do much about it.
But if, on the other hand, it is accessible via Class definition then THAT could indicate settings to use for:

  1. @JsonProperty(required = true) which has effect on Creator properties, and possibly (for 3.0), other properties
    2 @JsonSetter (as per above)

So I think we are on same page regarding finding mechanisms that can be used to support more natural handling from Kotlin POV.

@mikeholler
Copy link
Author

@cowtowncoder I have to pause on this for a bit today, but I want to partially reply to your comment so I can address the question about #250, which I think does apply:

    data class RecommendedSolution(
        @JsonSetter(contentNulls = Nulls.FAIL)
        val strings: List<String>
    )

    @Test
    fun `test recommended solution`() {
        expectThrows<JsonProcessingException> {
            mapper.readValue<RecommendedSolution>("""{"strings": ["a", null, "c"]}""")
        }
    }

The above test fails. Also, the following different annotation sites fail as well:

    data class RecommendedSolution(
        @field:JsonSetter(contentNulls = Nulls.FAIL)
        val strings: List<String>
    )
    data class RecommendedSolution(
        @param:JsonSetter(contentNulls = Nulls.FAIL)
        val strings: List<String>
    )
    data class RecommendedSolution(
        @property:JsonSetter(contentNulls = Nulls.FAIL)
        val strings: List<String>
    )
    data class RecommendedSolution(
        @get:JsonSetter(contentNulls = Nulls.FAIL)
        val strings: List<String>
    )

Note that @set: and @setparam:, which would make the most sense as annotation types for @JsonSetter don't exist (by design, since I'm using val not var). Any way to get the @JsonSetter behavior on a creator parameter instead of an actual setter?

@GedMarc
Copy link

GedMarc commented Oct 15, 2019

This may be my biggest shot in the dark ever

@NotNull is a validation api (), so jackson wouldn't catch it. Perhaps someone is feeling generous to build a data type for it, but for now, using the validation api/processor should suffice?
As long as your not using fong kong not null like ... i dunno.. org.jetbrains.annotations.NotNull, etc...
Something other than javax.annotation.NotNull


<dependency>   
 <groupId>org.hibernate.validator</groupId>   
 <artifactId>hibernate-validator-annotation-processor</artifactId>   
 <version>6.0.2.Final</version></dependency>

I think this may be a/the solution? As a processor is better than runtime execution but you can simply include it as well...

Everyones post is long so -

A quick note here is that hibernate-validator is entirely separate from the persistence aspects of Hibernate and by adding it as a dependency, we're not adding these persistence aspects into the project.

If you're in jdk 12 or up remember to provide your annotation to the annotation processor to your module-info.java

	provides javax.annotation.processing.Processor with bla bla bla;

The exception you should be catching is ValidationException as well, all of these will throw it

@Test
fun `list contains null`() {
    expectThrows<ValidationException> {
        val result = ObjectMapper().registerKotlinModule().readValue<ObjectWithStringList>("""{ "strings": ["a", null, "b"] }""")
    }
}
All of the annotations used in the example are standard JSR annotations:

    @NotNull – validates that the annotated property value is not null
    @AssertTrue – validates that the annotated property value is true
    @Size – validates that the annotated property value has a size between the attributes min and max; can be applied to String, Collection, Map, and array properties
    @Min – vValidates that the annotated property has a value no smaller than the value attribute
    @Max – validates that the annotated property has a value no larger than the value attribute
    @Email – validates that the annotated property is a valid email address

Some annotations accept additional attributes, but the message attribute is common to all of them. This is the message that will usually be rendered when the value of the respective property fails validation.

Some additional annotations that can be found in the JSR are:

    @NotEmpty – validates that the property is not null or empty; can be applied to String, Collection, Map or Array values
    @NotBlank – can be applied only to text values and validated that the property is not null or whitespace
    @Positive and @PositiveOrZero – apply to numeric values and validate that they are strictly positive, or positive including 0
    @Negative and @NegativeOrZero – apply to numeric values and validate that they are strictly negative, or negative including 0
    @Past and @PastOrPresent – validate that a date value is in the past or the past including the present; can be applied to date types including those added in Java 8
    @Future and @FutureOrPresent – validates that a date value is in the future, or in the future including the present

Quick read up : https://www.baeldung.com/javax-validation

@mikeholler
Copy link
Author

@GedMarc you're absolutely right, and I applaud your creativity; javax validation is exactly where I reached after having so many issues with Jackson. But sadly, we meet another impasse: meet KT-13228.

Kotlin does not yet support type annotations (which always surprises the heck out of me when I wait just long enough between uses to forget that that's the case). Looks like it's being added in 1.4, but it's not out yet 😢

https://youtrack.jetbrains.com/issue/KT-13228

@GedMarc
Copy link

GedMarc commented Oct 15, 2019

@mikeholler not sure if that is sarcastic or not xD either way...

Are you including the validation api?, or like hibernate or just referencing the java.activation library?
javax.validation allows a full run locally so this may just be a datatype subtype which requires the jaxb module

You could possibly get around this by creating a reader that simply throws out the validation?
Not sure if kotlin does jaxb?

This is a copy paste from my stuff, you can just throw it out in a reader.

List<String> errors = new ArrayList<>();
		ValidatorFactory factory = Validation.buildDefaultValidatorFactory();
		Validator validator = factory.getValidator();
		Set constraintViolations = validator.validate(entity);
		if (!constraintViolations.isEmpty())
		{
			for (Object constraintViolation : constraintViolations)
			{
				ConstraintViolation contraints = (ConstraintViolation) constraintViolation;
				String error = contraints.getRootBeanClass()
				                         .getSimpleName() + STRING_DOT + contraints.getPropertyPath() + STRING_EMPTY + contraints.getMessage();
				errors.add(error);
			}
		}
		return errors;

@GedMarc
Copy link

GedMarc commented Oct 15, 2019

  • Deserializer not reader..

wrong brain mode

@mikeholler
Copy link
Author

@GedMarc it's not sarcastic, it's a great idea but unfortunately not one I can use. We're using javax validation through Dropwizard's built-in model validation. I'd like it to work with that, but the lack of Kotlin type annotations got in the way.

@GedMarc
Copy link

GedMarc commented Oct 15, 2019

You don't need annotations this way :)
I'm not convinced it won't work in kotlin.
At the very least it does build a base for a datatype extension for you to include as a module

	@JsonDeserialize(using = ValidationAPIDeserializer.class)
	private String name;

It works for me, hmmm.


import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.DeserializationContext;
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
import com.fasterxml.jackson.databind.deser.std.StdDeserializer;

import javax.validation.ConstraintViolation;
import javax.validation.Validation;
import javax.validation.ValidationException;
import javax.validation.Validator;
import javax.validation.ValidatorFactory;
import javax.validation.constraints.NotNull;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.Set;

public class ValidationAPIDeserializer extends StdDeserializer<String> {

	protected ValidationAPIDeserializer() {
		super(String.class);
	}

	@Override
	public String deserialize(JsonParser p, DeserializationContext ctxt) throws IOException, JsonProcessingException, ValidationException {
		String value = p.getValueAsString();

		if (value == null || value.trim()
								  .isEmpty()) {
			return null;
		}
		return value;
	}

	@NotNull
	public List<String> validateEntity(Object entity) {
		List<String> errors = new ArrayList<>();
		ValidatorFactory factory = Validation.buildDefaultValidatorFactory();
		Validator validator = factory.getValidator();
		Set constraintViolations = validator.validate(entity);
		if (!constraintViolations.isEmpty()) {
			for (Object constraintViolation : constraintViolations) {
				ConstraintViolation contraints = (ConstraintViolation) constraintViolation;
				String error = contraints.getRootBeanClass()
										 .getSimpleName() + "." + contraints.getPropertyPath() + " " + contraints.getMessage();
				errors.add(error);
			}
		}
		throw new ValidationException(errors.toString());
	}
}

@cowtowncoder
Copy link
Member

@mikeholler Ok my bad on #250 then, I read it too quickly and somehow determined it was related to non-container properties of a data class.

But as to @JsonSetter, that should be applicable to Fields and Constructor parameters too. This was changed in 2.9, so perhaps you have dependency to older jackson-annotations?

Now, back to null-handling not working... I assume this class

src/main/kotlin/com/fasterxml/jackson/module/kotlin/KotlinValueInstantiator.kt

essentially replaces what BeanDeserializer would do. But that does not yet quite explain why handling of null-values would get overridden for individual JsonDeserializers. What matters there is that collection deserializers get configured with a NullValueProvider, and that is used to deal with values.
Unfortunately code in KotlinValueInstantiator is... quite complicated for me to understand.
But on plus side, codebase of module is quite small (compared to f.ex Scala module).

Another angle on null handling that might help is that instead of annotations, one can also use "config overrids" to specify defaults. Databind test src/test/java/com/fasterxml/jackson/databind/deser/filter/NullConversionsForContentTest.java has, for example:

    mapper.configOverride(List.class)
            .setSetterInfo(JsonSetter.Value.forContentNulls(Nulls.FAIL));

which should also achieve what you want for all properties with nominal (declared) type of List.

Or, for even more global applicability:

mapper.setDefaultSetterInfo(JsonSetter.Value.forContentNulls(Nulls.FAIL));

where one caveat is that "content" refers to Collection elements, Map entry values but NOT to Bean property values (which are "value" nulls, not "content").

I guess I am mostly just interested in figuring out how to make Kotlin module not block use of this setting: I do not think there is anything fundamental preventing it but maybe some missing wiring.
For example: for most property annotations to take effect, (de)serializers MUST be contextualized (call to createContextual()). It could be missing delegation somewhere.
Or, data class handler using set of BeanProperty objects pre-contextualization, not getting updated.

If Kotlin module had custom Collection deserializers, I would bet they were just missing handling of NullValueHandler, but I did not see any (if there are inner classes, maybe I just missed them).

@mikeholler
Copy link
Author

But as to @JsonSetter, that should be applicable to Fields and Constructor parameters too. This was changed in 2.9, so perhaps you have dependency to older jackson-annotations?

@cowtowncoder I can apply it to fields and constructors (that's what I did in my examples). They compile, Jackson just doesn't pick up how I'm trying to do it, even with the annotation site qualifiers like @field: and @param: shown above.

I really like the configOverride stuff, that's something I can probably implement. It'll become a problem when we implement the one or two JSON models where we actually want to allow nulls in lists (since we haven't been able to fine an annotation-based solution for Kotlin yet that allows behavior to be changed per model, rather than per mapper). It feels like every solution we find is so very close to being what we want, but just far enough to avoid feeling like a truly good solution.

@cowtowncoder
Copy link
Member

Ah. So I misunderstood that part wrt applicability.

One other thing: it'd be good to debug as to whether problem comes down to annotation not being merge to appropriate property (and so not visible to deserializer factory etc), or just not being used.
At this point I think I should be able to run a Kotlin test, if provided, and debug things from databind side.
So all I'd need is minimal case (smallest data class, trivial read), and I think I could be bit more productive.
I could also verify my understanding that Kotlin module still uses default CollectionDeserializer (... or not).

@mikeholler
Copy link
Author

Thanks @cowtowncoder, give me until EOB tomorrow to get you a small example project with tests that verifies that. I'll make it nice and easy for you to get going. Just to be clear, you want an example of the @JsonSetter problem and verification that the annotation is applied to the correct JVM site when compiled, right?

@cowtowncoder
Copy link
Member

For me test only needs to try to trigger expected behavior: it does not need to check bytecode or anything like that. I can observe (debugger or adding good old print statements :) ) what happens in places like creation and contextualization of CollectionDeserializer (and in general, kinds of deserializers even created) and see what that code sees.
I can also check out what JacksonAnnotationIntrospector finds, or, at even lower level, how AnnotatedClass (actual representation of collected annotations) get as their input.

Does this make more sense? The reason I want minimal case is just to reduce noise wrt resolution of things.

@mikeholler
Copy link
Author

Yeah for sure. I can absolutely put that together for you.

@mikeholler
Copy link
Author

@cowtowncoder in putting together what you asked, I ended up finding a solution to the problem. TL;DR, everything was fixed when I tried 2.10.0, which was completely unexpected but I am happy about it.

I am really curious if you might know what change between 2.9.10 and 2.10.0 fixed this behavior. I looked through so many issues, but none I could find that were relevant were merged in this time frame (or so I thought).

@cowtowncoder
Copy link
Member

That is an excellent outcome. I am not sure what might have fixed it... looking at release notes, I would guess that maybe #2280 did it? Or perhaps #2458. But difficult to know for sure.

One follow-up question: is use of this feature that you could explain on, say, README? I know how it works in general but often am not the best person to explain it. And in case of Kotlin there's more context too. But having a brief usage sample for commonly expected usage seems useful.

@mikeholler
Copy link
Author

@cowtowncoder you're probably right that it was one of those two. Thanks for those links.

One follow-up question: is use of this feature that you could explain on, say, README? I know how it works in general but often am not the best person to explain it. And in case of Kotlin there's more context too. But having a brief usage sample for commonly expected usage seems useful.

So, kinda? The tough part here is that I have this feeling that this should be the default behavior (failing fast and not allowing any nulls in a list by default, then overriding if nulls are needed is clearly the best outcome). It seems to me a large flaw in jackson-module-kotlin that it allows the behavior it does for nulls, which is completely unexpected for a Kotlin user. We could document a workaround to the flaw, or a plan could be developed to work to correct it. Unfortunately, correcting it would most definitely cause a breaking change to existing users, meaning a major version bump would be necessary.

At its simplest, I could document the @JsonSetter behavior in the README but I'm concerned how that will reflect on the library.

@cowtowncoder
Copy link
Member

It could be a separate Wiki page for repo too -- Workarounds (or something) for Kotlin?
I don't think it is unreasonable to explain how behavior currently (as of 2.10) differs from what might be expected; especially in that users may well bump into these problems without ways to alleviate the problem which is probably even worse impression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants