False positives

False positives

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

Some of these errors may 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 initialise 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 a JPA context an EntityManager annotated with @PersistenceContext is meant to be injected during application wiring at start-up. As long as the application is configured correctly, the application container ensures that em holds a reference to an EntityManager at run-time. Unfortunately, the Checker Framework can’t know this and there is no clean way of expressing it.

In this case, suppressing the error is the best option. We use the JDK’s @SuppressWarnings annotation and supply the appropriate suppression key 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.

Using assertions

Suppressing errors with @SuppressWarnings is the right approach in situations like the above, but there is something blunt about it. It’s good to explore other means to steer the checker towards the proper reasoning first.

The assert statement is a powerful but underused feature of Java. The purpose of an assert statement is to express some invariant, something known to be the case (unless our logic is faulty). Because the Checker Framework understands and honours propositions made with assert, it sometimes serves as an attractive alternative to @SuppressWarnings.

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 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 it 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. Since the ID of a database record cannot be null, neither can the ID of this pet. We can state this fact about the code with assert:

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

The error in JdbcOwnerRepositoryImpl.loadPetsAndVisits 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 a Map must not be null, but Owner.getId is annotated as being @Nullable.

Try tracking all incoming calls to loadPetsAndVisits in your text editor or IDE. You’ll 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. Again, 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 is a couple of real bugs. Give it a try squashing them yourself or follow me to the discussion on the next page, before we wrap up.