A bug discovered

A bug discovered

Another run of mvn -Pchecker install, another screen full of errors.

...
[INFO] 37 errors
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------

37 errors! This could mean that we're on the right track though: We're shaking out the suspicious null references.

Suspicious code

The first error is again in NamedEntity, and the suspicious code is in its toString method.

[ERROR] src/main/java/org/springframework/samples/petclinic/model/NamedEntity.java:
[47,28] [return.type.incompatible] incompatible types in return.
  found   : @Initialized @Nullable String
  required: @Initialized @NonNull String

@Override
public String toString() {
    return this.getName();
}

The author of this method intended 'named entities' like Pet and Owner to have their name as their string representation. But they forgot to think of a corner case that is now glaringly obvious for us: we just annotated getName as @Nullable – but toString() is not nullable and shouldn't be.

Imagine that somebody were trying to order pets by their string representation:

List<Pet> pets = ...
pets.sort(Comparator.comparing(Pet::toString));

It's a bit difficult to see, but this code would blow up with a NullPointerException as soon as there is a pet with no name in the list.

Fixing this is easy with the JDK's Objects.toString utility.

@Override public String toString() {
    return Objects.toString(getName());
}

As good citizens we're also covering the changed behaviour with a test. Check out commit HASH.

And another

The next error on the screen is in Owner:

[ERROR] src/main/java/org/springframework/samples/petclinic/model/Owner.java:
[137,28] [dereference.of.nullable] dereference of possibly-null reference compName

@Nullable
public Pet getPet(String name, boolean ignoreNew) {
    name = name.toLowerCase();
    for (Pet pet : getPetsInternal()) {
        if (!ignoreNew || !pet.isNew()) {
            String compName = pet.getName();
            compName = compName.toLowerCase();          // line 137
            if (compName.equals(name)) {
                return pet;
            }
        }
    }
    return null;
}

getPet is a public method and therefore a public API, and yet it makes assumptions about the current state of the object – which is mutable.

Not good. Clearly, if pet.getName() can return null, then calling toLowerCase on that reference may throw a NullPointerException. It's a bug.

The fix is easy, my solution is in commit HASH (again with a test).