A bug discovered

A bug discovered

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

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

Let’s take a look.

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 authors of this method intended ‘named entities’ like Pet and Owner to have their name as their string representation. Seems reasonable, but they forgot to think of a corner case that is now glaringly obvious for us: the name may be null. We’ve just annotated getName as @Nullable – but, as the diagnostic points out, toString is not nullable and shouldn’t be.

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

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

It might not be obvious, but the sort operation will blow up with a NullPointerException as soon as a pet with no name is encountered.

Fixing this is easy with the JDK’s Objects.toString method:

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

As good citizens we should also verify the changed behaviour with a test. I have done so in my fifth commit c4068e0.

And another

The next error on the screen is in Owner on line 137:

[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;
    }

A textbook case of a possible null dereference. Clearly, if pet.getName() can return null, calling toLowerCase on it is unsafe. The obvious thing to do is to add a null check, and that is what I did in my sixth commit 62d3c6d (again with a test):

                String compName = pet.getName();
                if (compName != null && compName.toLowerCase().equals(name)) {
                    return pet;
                }

That’s two actual NullPointerException bugs fixed!

A lot left to do but we’re going to take a shortcut one the next page.