Bugs squashed

Bugs squashed

There are a few remaining bugs to squash before we can wrap this 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 calls on nullable methods. We fix this 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 everywhere. Defocusing for a moment it is not hard to see that the Pet Clinic codebase doesn’t do particularly robust argument validation.

We commit this as the eleventh commit 2d795dd and move on. Five to go.

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 first expression, entity.getId() == entityId, compares Integer to int. What’s wrong with that?

Unboxing a boxed primitive can result in a NullPointerException. Don’t worry if you’re not familiar with this Java gotcha. 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 a NullPointerException.

This is another error that is ultimately due to 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 this as the replacement expression:

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

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

Success

I will pass over the small change required in PetController which I committed in commit 77ad340. (Bit tricky this one, 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 07d0b64. 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 reads return null;. states an return type incompatibility rather precisely. The method signature reads, with implied @NonNull added

protected @NonNull Integer mapForeignKey(@NonNull ResultSet rs)

A method that may return null is @Nullable and that’s what we need to make explicit by annotating it @Nullable. This is the fifteenth commit 1c3c1d6.

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 in our very first bugfix commit (ac6f454). 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 963645b.

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? 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. (mvn compile only compiles the production source.) Try resolving all errors in the tests too.

My solution is in the final three commits, commit 6f0ede7, commit ca07903, and commit 9586c6c.