Automagic (or not quite so) Domain Model Class Validation

Question

The Task

I was assigned a small task, concerning validation of Domain-Model classes.

The Validation for a String property of one of our Models was required to be unique across the whole Table. And as this already was halfway implemented after 20 minutes research, using a database constraint, it seemed fairly easy.

Obviously it wasn't, because what use does a validation have, when you're not able to show Error-Messages to your user in a feasible manner. The plan thusly changed slightly.

The only feasible approach to this seems to be a defensive select statement at persist time. This required me to use some additional features of JBoss, namely Seam and it's InjectingConstraintValidatorFactory. This allows me to access the current persistence context using an Injected EntityManager and run my defensive Select-Statement.

So here's what I ended up with:

@ManagedBean
public class UniqueLocationValidator implements
        ConstraintValidator<UniqueLocation, Location> {

    @Inject
    EntityManager entityManager;

    private String message;

    @Override
    public void initialize(final UniqueLocation annotation) {
        message = annotation.message();
    }

    @Override
    public boolean isValid(final Location instance,
            final ConstraintValidatorContext context) {
        if (instance == null) {
            // Recommended, instead use explicit @NotNull Annotation for
            // validating non-nullable instances
            return true;
        }

        final String checkedValue = instance.getLocationName();
        final long id = instance.getId();

        // must not return a result for name-equality on the same Id
        String queryString = "SELECT * FROM Location WHERE locationName = :value AND id <> :id";

        Query defensiveSelect = entityManager.createNativeQuery(queryString)
                .setParameter("value", checkedValue).setParameter("id", id);

        @SuppressWarnings("unchecked")
        List<Location> results = defensiveSelect.getResultList();
        if (!results.isEmpty()) {
            context.disableDefaultConstraintViolation();
            context.buildConstraintViolationWithTemplate(message)
                    .addNode("locationName").addConstraintViolation();
            return false;
        } else {
            return true;
        }
    }
}

Just for context, here's a stub version for the Location domain model-class:

@UniqueLocation
public class Location {

    @Id
    @GeneratedValue
    private long id;

    @Column(unique = true) //db-constraint
    @NotNull
    private String locationName;

    /* getters and setters */
}

Additionally the Annotation-Type given to the class is just the standard hibernate validation annotation:

@Target({ TYPE, ANNOTATION_TYPE })
@Retention(RUNTIME)
@Constraint(validatedBy = UniqueLocationValidator.class)
@Documented
public @interface UniqueLocation {

    String message() default "must be unique!";

    Class<?>[] groups() default {};

    Class<? extends Payload>[] payload() default {};
}

And here goes how it works.

At persist time, (after user presses Save) the Location runs through the JSF-Lifecycle, which includes a Validation of Field-Level Constraints. (@NotNull) Then it will get passed to the Data-Access-Layer of the project (Services).

There an EntityManager runs a persist (or update) against the persistence context. And here's where the magic happens.

The transaction manager will create a ConstraintValidatorFactory, and obtain a validator to run the Location against. Any Validation Failures or Exceptions lead to a transaction rollback. Validation errors additionally should throw a ConstraintViolationException. This Exception contains a Set<ConstraintViolation> which should get parsed by whoever requested the persist, to display error messages.

My concerns are two-fold:

1. Understandability
2. Idiomaticness

To expand a little. Especially since we got new staff in our project, and the complete project is only educational for complete programming newbies, I got the strong feeling, that they might be overstrained by the complexity of the underlying principles and the code.

Additionally this approach feels unclean, especially because the annotation and the validator can only validate a specific single Domain Model Class. Unfortunately for additional genericising I'd need to resort to hackish workarounds using reflection and prespecifying the unique columns.

Answer 1

• Querying "SELECT * FROM Location is a little over the head. You don't need to get all columns.

• As the value of queryString doesn't change, you should make it final

• Renaming message to constraintValidationMessage for understandability regarding new staff

• Single Responsible Principle is violated as you are querying a database and constructing a ConstraintViolation for the context.

  • Introduce a duplicateLocationExists() method which takes a Location as parameter and just returns a Boolean. The results object is only needed to check if it is empty, which should be returned by the duplicateLocationExists() method.

    private boolean duplicateLocationExists(final Location location){
    
        if (location == null) {
             // Recommended, instead use explicit @NotNull Annotation for
             // validating non-nullable instances
             return false;
        }
    
        final String checkedValue = location.getLocationName();
        final long id = location.getId();
        final String queryString = "SELECT id FROM Location WHERE locationName = :value AND id <> :id";
    
        Query defensiveSelect = entityManager.createNativeQuery(queryString)
            .setParameter("value", checkedValue).setParameter("id", id);
    
        return !defensiveSelect.getResultList().IsEmpty();
    }  
    

  • Inroduce a method for constructing the ConstraintViolation for the context

    private void createConstraintViolations(final ConstraintValidatorContext context){
        context.disableDefaultConstraintViolation();
        context.buildConstraintViolationWithTemplate(message)
               .addNode("locationName").addConstraintViolation();
    }
    

So the isValid() method will become

@Override
public boolean isValid(final Location instance,
        final ConstraintValidatorContext context) {

    if (duplicateLocationExists(instance)) {
        createConstraintViolations(context);
        return false;
    } else {
        return true;
    }
}