Bugs squashed

Bugs squashed

There are a few remaining bugs to squash before we can wrap up.

Run mvn -Pchecker compile and take a look at the errors around the middle.

Missing null checks

The half a dozen reports of ‘dereference of possibly-null reference’ in JdbcPetRepositoryImpl and JdbcVisitRepositoryImpl all look very much the same and can be dealt with swiftly.

Before:

    private MapSqlParameterSource createVisitParameterSource(Visit visit) {
        return new MapSqlParameterSource()
            .addValue("id", visit.getId())
            .addValue("visit_date", visit.getDate().toDate())
            .addValue("description", visit.getDescription())
            .addValue("pet_id", visit.getPet().getId());
    }

After:

    private MapSqlParameterSource createVisitParameterSource(Visit visit) {
        DateTime date = visit.getDate();
        Pet pet = visit.getPet();
    
        return new MapSqlParameterSource()
            .addValue("id", visit.getId())
            .addValue("visit_date", date == null ? null : date.toDate())
            .addValue("description", visit.getDescription())
            .addValue("pet_id", pet == null ? null : pet.getId());
    }

The authors of this code got lazy when they chained these method calls. The two methods in question are annotated @Nullable, therefore their return values may be null. We fix the broken code by adding the necessary null checks.

This is not an improvement to be too proud of – a better investment of time might have been to perform proper argument validation further up. From a wider angle, it is not hard to see that the Pet Clinic code isn’t particularly keen on robust argument validation.

We commit this as the eleventh commit b6b5c1b and move on.

Unboxing of null

Next on the list is a potential ‘unboxing of nullable’ in EntityUtils.getById on line 47:

[ERROR] src/main/java/org/springframework/samples/petclinic/util/EntityUtils.java:
[47,29] [unboxing.of.nullable] unboxing a possibly-null reference entity.getId()

        if (entity.getId() == entityId && entityClass.isInstance(entity)) {

The left-hand side of the conditional expression, entity.getId() == entityId, awkwardly compares Integer to int.

Well, unboxing a boxed primitive can result in a NullPointerException – a pitfall of Java (JLS 5.2). The == comparison here will be done in int, requiring unboxing of the Integer object. In case entity.getId() is null we will attempt to unbox null, earning us the NullPointerException.

This is another error that is ultimately caused by poor argument validation. Let’s not worry and simply make the comparison null-safe by boxing the primitive and then using equals instead of ==. I propose the following as a substitute:

Integer.valueOf(entityId).equals(entity.getId())

Committing this as the twelfth commit a41abc6, but not without making a mental note to write a test. Some day.

Success

I will pass over the small change required on line 80 of PetController, which I committed in commit 9f68177. (This one is tricky, can you explain why the change silences the error?)

The Checker Framework reports another plain-as-day possible null dereference in PetTypeFormatter.parse on line 59.

        for (PetType type : findPetTypes) {
            if (type.getName().equals(text)) {    // line 59
                return type;
            }
        }

The JDK offers Objects.equals for null-safe reference comparison, and it is just a perfect fit here.

-            if (type.getName().equals(text)) {
+            if (Objects.equals(type.getName(), text)) {

Committed as the fourteenth commit 659807b. Another NullPointerException averted.

It’s two left and we’re done.

[ERROR] src/main/java/org/springframework/samples/petclinic/repository/jdbc/JdbcPetVisitExtractor.java:
[44,20] [return.type.incompatible] incompatible types in return.
  found   : null
  required: @Initialized @NonNull Integer
[ERROR] src/main/java/org/springframework/samples/petclinic/web/PetTypeFormatter.java:
[53,31] [return.type.incompatible] incompatible types in return.
  found   : @Initialized @Nullable String
  required: @Initialized @NonNull String
[INFO] 2 errors

The statement on line 44 of JdbcPetVisitExtractor.mapForeignKey reads return null;. A return value null is definitely not compatible with the declared non-null return type Integer, read @NonNull Integer:

protected @NonNull Integer mapForeignKey(@NonNull ResultSet rs)

The method is @Nullable and that’s what must be made explicit by annotating it as such. This is my fifteenth commit 5398ab4.

So it has come to this: the final checker error. A vague notion of déjà vu strikes us as we glance at PetTypeFormatter.print:

    @Override
    public String print(PetType petType, Locale locale) {
        return petType.getName();
    }

Haven’t we fixed this bug before? We did fix a variation on the theme in our very first bugfix commit (c4068e0). There we used Objects.toString to coerce the nullable return type of getName to a non-null String. We can apply the same fix here and make this our sixteenth commit 52ffe51.

And with that, finally, mvn -Pchecker compile succeeds. We have reached ‘BUILD SUCCESS’. A 1600 lines-of-code codebase made null-safe – it wasn’t that much work after all.

Are we now bug-free? Wrap-up and some closing thoughts on the next page.

---

An exercise for the reader. mvn -Pchecker install still does not succeed, because we haven’t checked the test source yet. The compile Maven goal only compiles the production source.

Try resolving all errors in the tests too, by running mvn -Pchecker test-compile.

My solution is in the final three commits, commit 0741638, commit 46e6dbb, and commit 0d6666b.