5

Let's imagine, that we have a process, which accepts data of the following type:

{"date":"2014-05-05", "url":"http://some.website.com","counter":3}
  • This data should be validated formally: value of date should be a parseable date, url should also conform the normal url syntax.
  • Also, this data should be validated logically: date should be in the future, url should be an accessible address, returning 200 OK.

To make it clean, one must separate those two validation routines into different units (classes, utils, whatever). The desired final behaviour, however, must give user clear understanding of ALL violations, that are present in data. Something like:

{"Errors":[ "Specified date is not in the future",//Formal validation failed "Specified URL has invalid syntax"//Logical validation failed ]}

  • I have seen some implementations of the required behaviour, but they use those make use of Error objects and are full of checks like Error.hasErrors() or error==null, which does not look elegant.
  • I have also seen the implementation of javax.validation, which gives you all violations on all field at once. Same approach could be implemented for content validation, but I am not sure, that this is the best way to do this.

Question: what is the best practice for handling multiple exceptions/violations of various nature?

UPD: short digest of answers: collect Violations, build an Exception, containing their context, cause and description, use an interceptor to render. See reference links from answers:

http://beanvalidation.org/1.0/spec/ JSR 303 specification

http://docs.spring.io/spring/docs/3.2.x/spring-framework-reference/html/validation.html Spring Bean Validation

http://docs.oracle.com/javaee/6/tutorial/doc/gircz.html Java EE validation

Which Design Pattern To Use For Validation

Why not use exceptions as regular flow of control?

Community
  • 1
  • 1
Sergey
  • 427
  • 5
  • 12
  • there are two directions where this question may be going: (a) how to build arrange the code that implements the check; (b) how to arrange the code that aggregates the failures reported by these checks. Which direction is your question about? – Itay Maman May 13 '14 at 07:53
  • Both are equally important to me now. I suspect, that (b) influences the choice for (a) in some degree. Let us start with (b). – Sergey May 13 '14 at 07:57
  • take a look at this you might find it interesting: http://stackoverflow.com/questions/18314609/which-design-pattern-to-use-for-validation – maczikasz May 13 '14 at 07:59
  • You're right they are related to some extent. My answer (below) focuses mainly on collecting the results from a hierarchy of checks. – Itay Maman May 13 '14 at 08:19
  • 1
    This might help clarify why you should not use exceptions to control flow: http://stackoverflow.com/questions/729379/why-not-use-exceptions-as-regular-flow-of-control – Alexandre Santos May 13 '14 at 08:43

5 Answers5

3

I don't think there is a best practice, because it depends on what you try to achieve. In my opinion exceptions and their messages should not be used to be displayed directly to the user. Exceptions are much to technical and do depend heavy on the context where they get thrown. Hence my approach would be to design a container type which collects all the exceptions thrown by your validations. Those exceptions should preserve as much of the context as possible (not in form of an exception message, but in form of fields passed in to the constructor). Provide getter methods to make those fields (properties) accessable. When rendering the view you may iterate over all entries of your container and generate a proper, human readable, i18n message.

Here is some pseudo-code as requested by the comment of @Alexandre Santos. It is not polished nor profen, just my first draft. So do not expect excellent design. It's just an example how it could be implemented / designed:

public static void main(String[] args) {
    Violations violations = new Violations();
    Integer age = AgeValidator.parse("0042", "age", violations);
    URL url = UrlValidator.parse("http://some.website.com", "url", violations);
}

// Validator defining all the rules for a valid age value
public class AgeValidator {

    // Collection of validation rules for age values
    private static final Collection<Validator<String>> VALIDATORS = ...;

    // Pass in the value to validate, the name of the field
    // defining the value and the container to collect all
    // violations (could be a Map<String, ValidationException>)
    //
    // a return value of null indicates at least one rule violation
    public static Integer parse(String value, String name, Violations violations) {
        try {
            for (Validator<String> validator : VALIDATORS) validator.validate(value);
        } catch (ValidationException e) {
            violations.add(name, e);
        }
        return violations.existFor(name) ? null : Integer.parseInt(value);
    }
}
Harmlezz
  • 7,524
  • 23
  • 34
  • Well, there are many problem associated with this solution, which at the surface seems ok. One of such problems is consolidating many errors in the same exception. For example, suppose a field fails 2 validations (length and type for example), would you throw 2 exceptions (you couldn't). Then you have to throw one exception with a list of sub-exceptions. It gets messy really fast. – Alexandre Santos May 13 '14 at 08:08
  • I think you are able and should throw two exceptions. Each validation rule should be processed individually. It could be the responsibility of the exception container to group related exceptions. I would not throw one exception wit sub-exceptions, because the requirement to merge those information is motivated by the view and not by the validation layer. Hence never provide logic at a layer where no need can be motivated. It will not become messy in my opinion. It becomes clean and well separated. – Harmlezz May 13 '14 at 08:14
  • Could you paste some code where 2 exceptions are thrown at the same time? – Alexandre Santos May 13 '14 at 08:15
  • What I would like to avoid is passing here and there an object, containing violations. `violations.addAll(AgeValidator.validate("0042")); violations.addAll(UrlValidator.validate("http://some.website.com")); if(violations.NotEmpty) throw ExceptionFactory.newException(violations);` looks much more clean to me. – Sergey May 13 '14 at 08:55
  • If you do not like pass it in, then you have to return them. Either by throwing an exception containing all the rules violated or by returning a type which contains either the validated value or a collection of violations. It's up to you and your design how you like to treat the parsed / validated values, when you need them and how and when you want to address the violations / create the human readable messages. – Harmlezz May 13 '14 at 09:14
3

You can do the following:

define an abstract Check class, as follows:

public abstract class Check {
  private final List<Check> subChecks = new ArrayList<Check>();

  public Check add(Check subCheck) { subChecks.add(subCheck); return this }

  public void run(Data dataToInspect, List<Error> errors) {
    Error e = check(dataToInspect);
    if (e != null) {
       errors.add(e);
       return;
    }
    for (Check subCheck : subChecks) {
      subCheck.run(dataToInspect, errors);
    }
  }

  // Returns null if dataToInspect is OK.
  public abstract Error check(Data dataToInspect);
}

class Data is the class holding the data (that needs to be checked). Can be a String, a JSON object, what have you.

class Error represents a problem detected in the data should be roughly something like:

public class Error {
  private String problem;
  public Error(String problem) { this.problem = problem }
  public String getProblem() { return problem }
  // maybe additional fields and method to better describe the detected problem...
}

You then have code that runs the check against piece of data:

public class Checker {

   private final List<Error> errors = new ArrayList<Error>();
   private final List<Check> checks = new ArrayList<Check>();

   public Checker() {
     checks.add(new DateIsParsableCheck().add(new DateIsInTheFurutreCheck());
     checks.add(new UrlIsWellFormed().add(new UrlIsAccessible());

     checks.add();
     ..
   }

   public void check(Data d) {
     for (Check c : checks) {
       Error e = c.run(d, errors);
       if (e != null) 
         errors.add(e);
     }
   }
}

Slightly changed my original answer. In the current answer there is the notion of subchecks: if a check called x has a subcheck called y then the y check will run only if the x check succeeded. For instance, if the Date is not parseable there is no point to check it it is in the future.

In your case I think that all/most logical check should be sub-checks of a formal check.

Itay Maman
  • 28,289
  • 9
  • 76
  • 114
  • Would Checker need all possible checks done in the system added to it? – Alexandre Santos May 13 '14 at 08:10
  • How would it handle different date parsers? – Alexandre Santos May 13 '14 at 08:11
  • I would like to keep formal and logical checks apart, so I think of some constructor for `LogicalChecker`, which uses a list of formally valid fields (which I have previously validated with `FormalChecker`). I guess this still fits your approach. – Sergey May 13 '14 at 08:29
2

I have answered this previously Here

The answer marked as good is an example of the Composite pattern being applied to validation (almost)

There are, of course, tons of frameworks for this. Something clever you could do, that I have used to great effect, is to use an aspect + a validator or make sure whole swaths of new and existing code get checked auto-magically.

@Aspect
public class DtoValidator {

private Validator validator;

public DtoValidator() {
}

public DtoValidator(Validator validator) {
    this.validator = validator;
}

public void doValidation(JoinPoint jp){
    for( Object arg : jp.getArgs() ){
        if (arg != null) {
            Set<ConstraintViolation<Object>> violations = validator.validate(arg);
            if( violations.size() > 0 ){
                throw buildError(violations);
            }
        }
    }
}

private static BadRequestException buildError( Set<ConstraintViolation<Object>> violations ){
    Map<String, String> errorMap = new HashMap<String, String>();
    for( ConstraintViolation error : violations ){
        errorMap.put(error.getPropertyPath().toString(), error.getMessage());
    }
    return new BadRequestException(errorMap);
}
}

Here is a snip of bean config

<aop:config proxy-target-class="true">
    <aop:aspect id="dtoValidator" ref="dtoValidator" order="10">
        <aop:before method="doValidation"
                    pointcut="execution(public * com.mycompany.ws.controllers.bs.*.*(..))"/>
    </aop:aspect>
</aop:config>

Now all of your controller methods will have that validation code applied here and into the future.

Community
  • 1
  • 1
Christian Bongiorno
  • 3,923
  • 1
  • 25
  • 65
1

Designing it using exceptions will work, but you will have to write a whole framework to deal with exceptions, many of which can't be handled by your exception interceptor. If you feel the coding itch, then go for it. My advice would be to have different classes of exceptions. Some of them would be critical exceptions, some would be just warnings... you got the picture.

You could (I hope you do) use a proven framework that can handle that beautifully. I speak of JSR 303 and Bean Validation through Spring: http://docs.spring.io/spring/docs/3.2.x/spring-framework-reference/html/validation.html

It takes a while to get used to, but it will pay you back 1000 fold.

Alexandre Santos
  • 7,654
  • 7
  • 35
  • 60
  • The question was about general best practices, including situations, when one cannot use a framework. I do use `javax.validation` already, so some formal validation is already covered. Thank you for useful link, I will put it into original post. – Sergey May 13 '14 at 08:35
  • Yep. Saw that. Unfortunately there are no best practices. Just scars from previous battles. – Alexandre Santos May 13 '14 at 08:37
0

I would simply pass around a list of all the errors. The items in the list may not be just exceptions, but rather some objects wrapping more information about the errors, such as name of wrong parameter, its wrong value, position of the error in the string, type of validation (formal, ligical), ID of the error message for localized display to user... Each method on the processing path may append to the list.

Honza Zidek
  • 6,952
  • 4
  • 52
  • 82