[Feature request] Input data validation

[etki]

Hi! I think it would be terribly cool to add support for javax.validation and @Valid parameter annotation, which would automatically check for violations in deserialized entities and throw ConstraintViolationException in case of found violations.

[drejc]

Sure ... I can add this to the existing checks. If I'm not mistaken currently a IllegalArgumentException is thrown in case argument don't match up and a 400 bad request is thrown

[etki]

@drejc, yes, this totally works for things like @PathParam, i'm talking about deserializing request body {} as virtually any object (with null or default properties). Adding standard bean validation would help to process such case with a single ConstraintViolationException handler.

[drejc]

Will take a look ... my aim is to keep this project as lean as possible. Don't want to introduce unnecessary dependencies. But yes, sounds reasonable.

[juanavelez]

I second this enhancement. Currently I am doing it via Spring Validation but the error returned is a 500 error and the error message "Java.validation.ConstraintValidationException". It'd be great if these validation exceptions could be handled by anExceptionHandler so the correct HttpStatus error code and message could be sent back.

Very good work BTW on this, it really fills a need.

[drejc]

@juanavelez can you post a working example of how you are doing the validation. It should be possible to catch any exception produced.

[juanavelez]

I think I found the issue. I am using the following

    @Autowired
    private ExceptionHandler exceptionHandler;

        new RestBuilder(router)
            .register(loginController)
            .errorHandler(exceptionHandler)
            .build();

It looks like when using an instance of ExceptionHandler (as opposed to a Class<?>) the following is called

    public final void register(ExceptionHandler... handlers) {

        Assert.notNullOrEmpty(handlers, "Missing exception handler(s)!");
        for (ExceptionHandler handler: handlers) {

            Assert.isFalse(ContextProviderFactory.hasContext(handler.getClass()),
                           "Exception handler utilizing @Context must be registered as class type not as instance!");

            Type generic = getGenericType(handler.getClass());
            Assert.notNull(generic, "Can't extract generic class type for exception handler: " + handler.getClass().getName());
            super.register((Class)generic, handler);
        }
    }

which in turn calls

    protected void register(Class<?> aClass, T instance) {

        Assert.notNull(aClass, "Missing associated class!");
        Assert.notNull(instance, "Missing instance of class!");

        if (checkCompatibility(instance.getClass())) {
            Type expected = getGenericType(instance.getClass());
            checkIfCompatibleTypes(aClass,
                                   expected,
                                   "Incompatible types: '" + aClass + "' and: '" + expected + "' using: '" + instance.getClass() + "'");
        }

        cache.put(aClass.getName(), instance);
    }

But when determining what ExceptionHandler to use

The lookup in the cache is lower priority than the default ones

        // get by exception type from classTypes list
        if (found == null) {
            found = super.get(aClass);
        }

        // nothing found provide generic
        if (found == null) {
            found = GenericExceptionHandler.class;
        }

        return super.getClassInstance(found, provider, context);

I wonder if it should be

    public final void register(ExceptionHandler... handlers) {

        Assert.notNullOrEmpty(handlers, "Missing exception handler(s)!");
        for (ExceptionHandler handler: handlers) {

            Assert.isFalse(ContextProviderFactory.hasContext(handler.getClass()),
                           "Exception handler utilizing @Context must be registered as class type not as instance!");

            Type generic = getGenericType(handler.getClass());
            Assert.notNull(generic, "Can't extract generic class type for exception handler: " + handler.getClass().getName());
                        // super.register((Class)generic, handler);
            exceptionHandlerInstances.add(handler); // Register the instance directly
        }
    }

[drejc]

Cool will check ...

[juanavelez]

@drejc I noticed you made a change to way error handling works. (0.8.4-RC1). I tried it but I am not sure if I understand correctly how the exception handling works. If I create an ExceptionHandler whose type argument is Throwable, will it be used for all instances of Throwable or just for exceptions whose class match java.lang.Throwable? I was hoping it would be the former but it seems to be the latter?

The use case is the following. I have created an abstract base Exception (e.g. MyAbstractBaseException extends RuntimeException) from which there will be many (up to hundreds) of subclasses. Instead of creating an ExceptionHandler for each class, I would hope that creating an ExceptionHandler would be enough to handle all exceptions whose base class is MyAbstractException.

I know it would be seem like an issue to handle hierarchies: let's say Class A extends RuntimeException, Class B extends A, Class C extends B, and if we create ExceptionHandler< A >, ExceptionHandler< B > and ExceptionHandler< C > and you try to match C, which one you'd call? In this case, I'd say to match to the closest one, which would be ExceptionHandler< C >. If you had the same exception hierarchy but there would be only ExceptionHandlers for A and B, then ExceptionHandler< B > would be the one to use for C as it would be the closest one.

[drejc]

@juanavelez yes I'm refactoring the exception handling part as it is a little bit messy, so stay tuned.

As to how exception handling now works is it tries to match the thrown exception with the registered exception handlers in the order handlers have been registered.

Exception handlers are used as listed ... first one matching the exception will be used.

register(MyExceptionHandler.class, IllegalArgumentExceptionHandler.class, WebApplicationExceptionHandler.class, ...);

This works for class type exceptions ... but I think there is a bug and it will not work correctly for instant types, as I turns out I had unit tests covering this but the tests were incorrect :(

Additionally you can override this with a specific exception handler with the @CatchWith annotation that will be used first on the given REST.

Now I haven't tested how it behaves if you derive exception handlers from other exception handlers, it might be that this will not work correctly. But I will check it.

However in the grand scheme of things it might not be a great idea to have a lot of exception handlers, but rather only a few, and do the additional logic inside the handler if necessary.

[juanavelez]

@drejc Hello, I see you made some changes to the code to handle the exception mapping better. I just tried with the latest code and the ExceptionHandler instance I registered is still not being called

public class GlobalExceptionHandler implements ExceptionHandler<Throwable> {
    @Override
    public void write(Throwable result, HttpServerRequest request, HttpServerResponse response) {
        System.out.println("Insider GlobalExceptionHandler.write method");
        response.setStatusCode(getStatusFromException(result));
        response.end(result.getMessage());
    }

...
}

ExceptionHandler handler = new GlobalExceptionHandler();

...
        new RestBuilder(router)
            .register(loginController)
            .errorHandler(exceptionHandler)
            .build();

In my controller code, I am raising the following:

 throw new RuntimeException("Invalid login");

Following the code, it seems the registration is indeed putting GlobalExceptionHandler.class mapped to the appropriate instance in the cache of ClassFactory. However when processing the RuntimeException, it fails to find it, mostly because it is trying to get from the cache the class of the caught exception (RuntimeException).

            ExceptionHandler handler = getCached(aClass.getName());

But it finds it as

        // get by exception type from classTypes list
        if (found == null) {
            found = super.get(aClass);

However, this leads to creating an instance of the class, which is not what it is needed (what it's needed is to use the instance that what registered). IMHO, I think whatever you are doing with super.get(aClass) would need to be applied to getCached, this is, the code that tries to find the closes match

        // try to find appropriate class if mapped (by type)
        for (Class key : classTypes.keySet()) {
            if (key.isInstance(type) || key.isAssignableFrom(type)) {
                return classTypes.get(key);
            }
        }

[drejc]

@juanavelez it's work in progress ... the problem is finding the correct cached class no matter the exception thrown ... currently there is only a simple mapping exception type - handler when the instance is cached/registered.
Now if you throw a inherited exception the instance will not be found ... as you are describing. I need to create a better way of storing multiple keys (exceptions) bound to a single handler ... then this issue will be sorted.

[drejc]

ConstrainViolation support now available in 0.8.4 version