More precise exceptions

[erickskrauch]

Each verifier throws it's own Exception. Messages for now stored inside Exception classes.
Also I fixed some unused imports and type hinting for IDEA editors.

[emarref]

I'm guessing your reasons for introducing these extra exception classes is to ease exception handling. You might like to catch different validation exceptions and handle them differently.

I have intentionally not introduced specific classes for validation exceptions, as I believe the reason to use different classes for exceptions is focussed on how the error should be handled, not what caused the error. An exception class is a class - a general type of thing - e.g. failed validation. It is not an ID - e.g. specific instance of thing.

For instance, in the case of calling an API, an example might be receiving a 404 response vs a 503 response from a third party service. In these cases, application code should react very differently. I would expect to see two catch blocks for the different exceptions. The 404 is likely a permanent problem, but the 503 might be attempted again at some point in the future due to temporary service failure.

In the case of validation, application code should react very similarly in all cases of validation failure. Any differences in how the code reacts to a validation exception should be handled by a switch statement on the $exception->getCode()error code in a catch block, but in the long run, the purpose of catching these exceptions is to surface helpful resolution information to the user.

Or consider another example; the catch block might sit pretty high up in the order of execution - perhaps in the controller. A controller would care if a validator couldn't connect to a database, and it would be handled very differently to different types of validation failure.

I'd be interested to hear your opinion on this. I'm open to a change of mind if you have a good argument.

[erickskrauch]

Sorry for such long wait for a response.

Let me show, how I use this. I'm using Yii2.

Before:

try {
    $jwt->verify($token, $context);
} catch (VerificationException $e) {
    if (StringHelper::startsWith($e->getMessage(), 'Token expired at')) {
        $message = 'Token expired';
    } else {
        $message = 'Incorrect token';
    }

    throw new UnauthorizedHttpException($message);
}

After (with my PR):

try {
    $token = $component->parseToken($token);
} catch (ExpiredException $e) {
    throw new UnauthorizedHttpException('Token expired');
} catch (VerificationException $e) {
    throw new UnauthorizedHttpException('Incorrect token');
}

All new Exceptions extends base \Emarref\Jwt\Exception\VerificationException, so all exists applications will save their behaviors, but when they will need a more exact cause of validation failure, they will find it by using more precise Exception class (like I did it in my app).

So I can still offer to release version 1.1.0 instead increment the patch number, because changes back-compatible, but bring new features.

[emarref]

So all code aside, what you're trying to achieve is three things:

1. In all cases, return a custom HTTP exception
2. Customise the expired message
3. Default to generic message if not expired.

My hesitations on providing multiple exception classes still stands. All verification exceptions are handled the same way, which would suggest multiple catch blocks are not the answer in this situation. This use-case is what exception codes were designed for. Your example code would violate the DRY principle by throwing the same exception for similar reasons from multiple places.

I've put together a pull request to show what I mean - see #19. Using the exception codes feature above, your code should look like this:

try {
    $token = $component->parseToken($token);
} catch (VerificationException $e) {
    switch ($e->getCode()) {
        case VerificationException::CODE_EXPIRED:
            $message = 'Token expired';
            break;
        default:
            $message = 'Incorrect token';
            break;
    }

    throw new UnauthorizedHttpException($message);
}

What are your thoughts on this approach?

[erickskrauch]

I don't like you solution with consts and codes.

My solution allows me to get additional info from ExpiredException and TooEarlyException. Also, my solution allows decrease nesting level of exception handler. Also, my solution allows to handle only one error and allow other exceptions float above call stack. (I hope, you understand my last sentence, because I'm not sure, that I translated it correctly).

The fact that I use in my project provided code with same exceptions and different messages today does not mean that in the future I do not want to make a more specific error handling. Expired token for my app is ok. We use refresh tokens. But I want to log all other failures to detect hacking attempts, for example. So... With you solution with codes I will have ugly switch with unjustified level of nesting.

Exceptions was introduced to help developers handle exception (lol, amazing!) and they can be grouped by some base exception class to provide developers ability to check all exceptions of same type or handle exact error by catching precise exception.

Look at the another implementation. Code is not so cool, but they use different exceptions for different validation errors. And you're up against?

[emarref]

I'm still personally against the implementation of multiple exceptions for this purpose. But given there is no immediate reason not to implement this if it helps other people's workflow, it looks like a good option to include.

Thanks very much for your contributions.

[erickskrauch]

Thanks! :)