False positives

False positives

We have silenced the errors in foreign code. 17 errors remain, and they should all be in the Pet Clinic’s application code.

However, some of the errors reported in our own code may also be false positives that we want to dismiss. For this we have basically two means: suppressing the errors, and assertions.

Suppressing errors

Scanning over the remaining errors you’ll notice a few saying the constructor does not initialize fields. For example, JpaPetRepositoryImpl appears not to initialise the em field.

[ERROR] src/main/java/org/springframework/samples/petclinic/repository/jpa/JpaPetRepositoryImpl.java:
[38,8] [initialization.fields.uninitialized] the constructor does not initialize fields: em

@Repository
public class JpaPetRepositoryImpl implements PetRepository {

    @PersistenceContext
    private EntityManager em;

    @Override
    public Pet findById(int id) {
        return this.em.find(Pet.class, id);
    }

    // ...

}

Indeed, em is not initialised, but it must also be non-null or else the call to em.find in findById could never succeed.

Well, we know that in JPA, which these repository classes use, an EntityManager annotated with @PersistenceContext is meant to be injected at run-time during application wiring at start-up. We know that as long as the application is configured correctly, the application container will ensure that em holds a reference to an EntityManager at run-time. Unfortunately, the Checker Framework doesn’t know this and there is no clean way of expressing this.

In this case, suppressing the error is the best option. We can use the JDK @SuppressWarnings annotation and supply the name of the checker as the value (the initialization checker is part of the Nullness Checker).

    @PersistenceContext
    @SuppressWarnings("initialization")
    private EntityManager em;

I’ve made this change to all four injected EntityManager instances in the ninth commit b5443b5. With that we are down to 13 errors.

Using assertions

Suppressing warnings is really the only option in situations like the above, where the programmer knows something about the application that the Checker cannot. However, it’s good to explore other means to steer the Checker towards the proper reasoning before applying the @SuppressWarnings chloroform.

The assert statement is a useful but underused feature of Java. assert is usually used to express some invariant, something known to be true (unless our logic is faulty). The Checker Framework understands and honours propositions made with assert.

When using the Checker Framework, I find assert works best when combined with the -AassumeAssertionsAreEnabled flag. Without this flag, the Checker Framework only considers assert statements that have a special "@AssumeAssertion(...)" string key as the message. Personally, I prefer to forgo this special assertion message device and simply assume that assertions are enabled. (I also run all my JVMs with -ea but that may be just me.)

Let’s see how this works. Open JdbcPetRepositoryImpl on line 100.

        try {
            Map<String, Object> params = new HashMap<>();
            params.put("id", id);
            pet = this.namedParameterJdbcTemplate.queryForObject(
                "SELECT id, name, birth_date, type_id, owner_id FROM pets WHERE id=:id",
                params,
                new JdbcPetRowMapper());
        } catch (EmptyResultDataAccessException ex) {
            throw new ObjectRetrievalFailureException(Pet.class, id);
        }

        // line 100:
        List<Visit> visits = this.visitRepository.findByPetId(pet.getId());

[ERROR] src/main/java/org/springframework/samples/petclinic/repository/jdbc/JdbcPetRepositoryImpl.java:
[100,72] [argument.type.incompatible] incompatible types in argument.
  found   : @Initialized @Nullable Integer
  required: @Initialized @NonNull Integer

The Checker complains that pet.getId() is nullable, but findByPetId does not accept a null argument. Inspecting the code we realise that pet at this point will always be a Pet instance populated from the database, so it must have an ID – the ID cannot be null. We can express this knowledge about the code with an assert statement:

        Integer petId = pet.getId();
        assert petId != null;
        List<Visit> visits = this.visitRepository.findByPetId(petId);

The error in JdbcOwnerRepositoryImpl on line 112 is similar.

    public void loadPetsAndVisits(final Owner owner) {
        Map<String, Object> params = new HashMap<>();
        params.put("id", owner.getId());                  // line 112
        // ...
    }

By default, values in Map must not be null, but owner.getId is annotated to be @Nullable.

Try to track all incoming calls to loadPetsAndVisits in your text editor or IDE. You will find that this method is used only locally (make it private), and on all paths leading to it, owner has been populated with an owner from the database. Database primary keys are definitely not null, so again we are confident that getId cannot ever return null:

    private void loadPetsAndVisits(final Owner owner) {
        Integer ownerId = owner.getId();
        assert ownerId != null;
        Map<String, Object> params = new HashMap<>();
        params.put("id", ownerId);
        // ...
    }

The final error in this batch is in OwnerController on line 90. Blindingly obvious for us but the Checker needs a little help.

        // ...
        if (owner.getLastName() == null) {
            owner.setLastName("");
        }

        Collection<Owner> results = this.clinicService.findOwnerByLastName(owner.getLastName());
        // ...

As before, owner.getLastname carries the @Nullable annotation, but findOwnerByLastName does not accept a null argument. Clearly, the if (owner.getLastname() == null) guard block has just ruled out a null last name, but the Checker does not understand the special relationship between the getters and setters. Once we’re past the owner.getLastName() == null guard an assertion that the last name is now not null is necessary.

I have committed all this in the tenth commit da8d9aa.

All that remains now are a couple of real bugs, ten to be precise.

Give it a try squashing them yourself or follow me to the discussion on the next page, before we wrap up.