2

Observe the Following Code. A DealerLot has a carInventory, which consists of cars, all of which contain an engine. The current dealerlot has two cars in inventory, and when validated one of the car's engines fails validation. However the error message only contains information about the engine. What would be the cleanest way of including the carName in the message as well?

public class main {

@Data
@AllArgsConstructor
static class Engine {
    @Pattern(regexp="V.*", message="Engine Name must start with a V! You provided ${validatedValue}")
    private String engineName;
}

@Data
@AllArgsConstructor
static class Car {
    private String carName;
    @Valid
    private Engine engine;
}

@Data
@AllArgsConstructor
static class DealerLot {
    @Valid
    private List<Car> carInventory;
}

public static void main(String args[]){
    ValidatorFactory factory = Validation.buildDefaultValidatorFactory();
    Validator validator = factory.getValidator();

    Engine engine1 = new Engine("VQ35DE");
    Car car1 = new Car("Nissan 350z", engine1);
    Engine engine2 = new Engine("2JZ-GTE");
    Car car2 = new Car("Toyota Supra", engine2);
    DealerLot dealerLot1 = new DealerLot(Arrays.asList(car1, car2));

    Set<ConstraintViolation<DealerLot>> i = validator.validate(dealerLot1);
    for (ConstraintViolation<DealerLot> v : i) {
        System.out.println(v.getMessage());
    };
}}

Output:

Engine Name must start with a V! You provided 2JZ-GTE

I would prefer output to look somewhat like the following:

Toyota Supra, Engine Name must start with a V! You provided 2JZ-GTE

Without this level of clarity it could be extremely confusing to determine which car(s) are having an issue.

FYI these are the version of the dependencies I am using in the example:

compile group: 'org.hibernate.validator', name: 'hibernate-validator', version: '6.0.17.Final'
compile group: 'org.glassfish', name: 'javax.el', version: '3.0.1-b11'
implementation group: 'org.projectlombok', name: 'lombok', version: "1.18.4"
compile group: 'javax.validation', name: 'validation-api', version: '2.0.1.Final'
Copy and Paste
  • 496
  • 6
  • 16
  • You could probably add a backreference from Engine dto to Car dto, and then use expression language in the message to fetch the name via that reference. I haven't tried such things though. – vadipp Dec 04 '19 at 15:34
  • 1
    This is an aggregation problem. If you want the parent class (`Car`) to report an issue with one of its components (`Engine`), then you need to move the validation there. – hfontanez Feb 14 '22 at 17:59

1 Answers1

1

Like hfontanez said, you need to move the validation to Car. You can still get the error reported on the engine's name using a custom validation annotation (let's call it ValidCarEngine) and validator.

public class ValidCarEngineValidator implements ConstraintValidator<ValidCarEngine, Car> {

private static final Pattern ENGINE_NAME_PATTERN = Pattern.compile("V.*");

@Override
public boolean isValid(Car car, ConstraintValidatorContext context) {
    if (car == null || car.getEngine() == null || car.getEngine().getEngineName() == null) {
        return true; // use @NotNull if needed
    }
    String carName = car.getCarName();
    String engineName = car.getEngine().getName();

    if (Pattern.matcher(engineName).matches()) {
        return true;
    }
    // Disable the out-of-the-box error on the Car
    constraintValidatorContext.disableDefaultConstraintViolation();

    String message = carName + ", Engine Name must start with a V! You provided " + engineName;
    constraintValidatorContext.buildConstraintViolationWithTemplate(message)
            .addPropertyNode("engine")
            .addPropertyNode("engineName")
            .addConstraintViolation();
    return false;
}

This uses a hard-coded pattern for all cars, but you can make even add a mapping from car type/name to pattern (just make sure to use a default, or return true if there is no mapping).

Rob Spoor
  • 6,186
  • 1
  • 19
  • 20
  • This is why I added the concept of a dealer lot. So no matter how deep the attribute tree goes, in this case we are 3 deep (Engine -> Car -> DealerLot), you would write a custom ConstraintValidators to individually validate each sub attribute? Even if we were say 30 attributes deep? It seems a bit off to me that we MUST create car level validation for validating an engine. In addition you would have duplicate code for validating the engine within the `Engine` class itself, and the `ValidCarEngineValidator `. Of course, you could extract this out... But these seem like simple pojos. – Copy and Paste Feb 22 '22 at 21:21
  • @CopyandPaste you only need to have a car level validation for validating an engine if you need anything from the car in the engine validation. That includes the car's name. – Rob Spoor Feb 23 '22 at 12:02
  • Why should car level validation be handling engine validation? The Regex pattern is specific to engine validation itself. Take for example, a pallet engine (not yet installed in a car); we may want to validate this engine itself. Using your code above I would have to create a faux car, in order to validate the engine. Unless you are suggesting we duplicate the validation? – Copy and Paste Feb 24 '22 at 01:52
  • @CopyandPaste because you want to use a part of the car inside the validation. It's only the message, but still. – Rob Spoor Feb 24 '22 at 08:59
  • Furthermore, if what you are claiming is correct I question the value of the constraint annotations, such as `@Pattern` (`javax.validation.constraint` ). Essentially anytime you have any class that is composed with other classes, it is a risky decision to use the default annotations. It would appear for even the simplest of POJOs we should all be using custom `ConstraintValidator`s – Copy and Paste Apr 05 '22 at 23:12
  • If `@Pattern` (and others) works just fine with the hard-coded annotation values there is no need for a custom validator. In your case it would be needed because the hard-coded annotation values are not sufficient - you want dynamic messages. The only thing that is dynamic to the hard-coded message template is the resolving of the annotation's own properties etc. inside the templates, like the `validatedValue` you use. Anything else cannot be retrieved from just the annotation and annotated element themselves. – Rob Spoor Apr 06 '22 at 07:03
  • I'd disagree with your first sentence, as you may be lured into a false sense of security that it "works just fine". It works fine up until the moment you need to accommodate an enhancement such as the custom message I posed. To accommodate that enhancement a massive refactoring would be required, as annotations such as `@Pattern` are no longer re-usable, and custom `ConstraintValidator`s must be used to replace them. – Copy and Paste Apr 06 '22 at 16:45
  • That's true, but I can probably count the number of custom validators I've written just to get the message or property path correct on 1 hand. All other cases contained actual logic. – Rob Spoor Apr 06 '22 at 17:15