Java 9 Immutable List Nullpointer

[28Smiles]

public final This defineList(String key, List<?> values) {
        if (values.isEmpty()) {
            throw new IllegalArgumentException(
                    getClass().getSimpleName() + ".defineList was called with an empty list.");
        }
        if (values.contains(null)) {
            throw new IllegalArgumentException(
                    getClass().getSimpleName() + ".defineList was called with a list containing null values.");
        }

        String value = values.stream()
                .map(Object::toString)
                .collect(joining(", "));

        return define(key, value);
}

values.contains(null) throws a nullpointer exception if list is created with List.of(), since it's contains method produces a NullpointerException if filled with null:

    @Override
    public boolean contains(Object o) {
        for (E e : elements) {
            if (o.equals(e)) { // implicit nullcheck of o
                return true;
            }
        }
        return false;
    }

[qualidafial]

List.of() API docs say that NPE will be thrown for any null elements: https://docs.oracle.com/javase/9/docs/api/java/util/List.html#of-E-

[28Smiles]

@qualidafial yes thats right, but in case of contains, the List will throw a NPE if asked if it contains null

[qualidafial]

Do you want Jdbi to stop calling contains() because one implementation doesn't guard against null?

[leaumar]

This very much sounds like a non-jdbi-issue to me, for what my opinion's worth. It's not jdbi's fault the list decides to throw an npe on null items it contains (why can you even get a null into it if it doesn't like nulls?). Jdbi needs to check for nulls, so it does. Your issue is with the list implementation you decided to use.

[28Smiles]

@TheRealMarnes @qualidafial I agree it isn't an jdbi issue, but isn't it desireable to at least support all types of list the JCL provides, a fix would be to do it simmilar to the bindList implementation, with a stream.

[leaumar]

This List.of that throws an npe on contains when it has null items (that you still got into the list in the first place) and doesn't throw that npe when you stream the items (because logic) is seriously a jdk implementation?

I mean, jdbi kinda has to support it then, but I can't wrap my head around such a thing actually being in the jdk.

[28Smiles]

@TheRealMarnes Yes it is, that is why I opened this Issue.

[leaumar]

I'm in favor of a workaround in jdbi then, but it'll be with an angry look toward the jdk and a code comment explaining that the relevant code in jdbi has no right to exist then ¯\_(ツ)_/¯

[28Smiles]

I will open a pull request then

[stevenschlansker]

Thanks for tracking this down, everyone :)

[dhardtke]

@TheRealMarnes You got something wrong there: The List.of factory method creates immutable List instances that do not permit null elements.

The Javadoc is clear on the behavior of java.util.List#contains(java.lang.Object):

Throws NullPointerException - if the specified element is null and this list does not permit null elements (optional)

So in my opinion this is a concern on the side of Jdbi and not an issue in the JDK itself - the JDK team made this decision (for some reason) and everyone has to stick to it.

[qualidafial]

Yeah, I just noticed that the optional throws clause has been there since Java 5. 🤢

Still think it breaks LSP but it's in the spec so we'll deal with it