Obj#setActiveGroupNames() fails if used on an immutable collection

[Igrium]

The method Obj#setActiveGroupNames() fails with a NullPointerException if used on a collection generated by Set.of() or List.of(). This is because those collections throw an exception if you try to test if they contain null, which you do when in the block:

else if (groupNames.contains(null))
{
    throw new NullPointerException("The groupNames contains null");
}

This is pretty annoying because if you're trying to add a face to only one group, that's what you'll be using 90% of the time.

[javagl]

It does not generally fail on an immutable collection, but only on one that does not permit null elements (and does the corresponding check in contains).

And... what should happen when someone does call this function with a null element?

One can write

obj.setActiveGroupNames(Arrays.asList("a", "b"));

or use Collections#singleton* for single elements, so I don't think that's an issue.

[Igrium]

Still worth noting as a bug; it shouldn't have taken me digging through the source code to figure it out. A simple try/catch in the implementation would fix it.

[javagl]

The documentation of that method says that it will throw a NullPointerException when the collection contains null elements. It's reasonable to assume that it does so by checking whether the collection contains null elements, right? (I rather find the fact that set.contains(null) throws an NPE a bit obscure).

A simple try/catch in the implementation would fix it.

I'm curious what your suggested "simple" solution would look like.

[Igrium]

Something along these lines?

    @Override
    public void setActiveGroupNames(Collection<? extends String> groupNames)
    {
        if (groupNames == null)
        {
            return;
        }
        if (groupNames.size() == 0)
        {
            groupNames = Arrays.asList("default");
        } else {
            boolean containsNull = false
            try {
                containsNull = groupNames.contains(null)
            } catch(NullPointerException e) {}
            if (containsNull) {
                throw new NullPointerException("The groupNames contains null");
            }
        }
        nextActiveGroupNames = 
            Collections.unmodifiableSet(new LinkedHashSet<String>(groupNames));
    }

(I wrote this without an IDE so the syntax may be wrong)

[javagl]

It was that boolean and the empty catch block that I wondered about, and is hard to justify.

Just using obj.setActiveGroupNames(Arrays.asList("example")); should be fine.

[javagl]

Instead of that try-catch, I'd rather consider something like

for (String groupName : groupNames) {
    if (groupName == null) throw up;
}

for the check,

But I'm still a bit baffled by the NPE that Set#of is throwing for contains(null). I do not see any reason whatsoever (and not even the slightest technical justification) to not just return false in this case...