Split parsing and checking JSON-API headers into two

[philbates35]

To replicate:

• Fresh clone of this repo (at the time of writing the latest is e029c43).
• Disable authentication by commenting out the middleware:

Route::group([
    'prefix'     => 'api/v1',
    'middleware' => [
//        Kernel::JSON_API_JWT_AUTH, // comment out this line if you want to disable authentication
    ]
]
...

• Throw a Validation Exception in the any controller method, e.g.:

class AuthorsController extends JsonApiController
{

/**
  * Display a listing of the resource.
  *
  * @return Response
  */
public function index()
{
    throw new \Illuminate\Contracts\Validation\ValidationException(
        new \Illuminate\Support\MessageBag([
            'first_name' => 'First name must be provided',
            'last_name' => 'password must be 7 characters'
    ]));
    // $this->checkParametersEmpty();
    // return $this->getResponse(Author::all());
}

• Visit the URI corresponding to the controller method you threw the ValidationException in (in my example this is api/v1/authors).

A 500 HTTP code is returned with no response content. The full trace in the laravel.log is:

[2015-10-22 13:46:33] local.ERROR: exception 'Symfony\Component\Debug\Exception\FatalErrorException' with message 'Call to a member function encodeErrors() on null' in /var/www/vhosts/limoncello-collins/vendor/ neomerx/limoncello/src/Errors/ConvertContentRenderer.php:68
 Stack trace:
 #0 /var/www/vhosts/limoncello-collins/bootstrap/cache/compiled.php(2088): Symfony\Component\Debug\Exception\FatalErrorException->__construct()
 #1 /var/www/vhosts/limoncello-collins/bootstrap/cache/compiled.php(2083): Illuminate\Foundation\Bootstrap\HandleExceptions->fatalExceptionFromError()
 #2 /var/www/vhosts/limoncello-collins/bootstrap/cache/compiled.php(0): Illuminate\Foundation\Bootstrap\HandleExceptions->handleShutdown()
 #3 /var/www/vhosts/limoncello-collins/vendor/neomerx/json-api/src/Exceptions/BaseRenderer.php(156): Neomerx\Limoncello\Errors\ConvertContentRenderer->getContent()
 #4 /var/www/vhosts/limoncello-collins/app/Exceptions/Handler.php(116): Neomerx\JsonApi\Exceptions\BaseRenderer->render()
 #5 /var/www/vhosts/limoncello-collins/bootstrap/cache/compiled.php(2320): App\Exceptions\Handler->render()
 #6 /var/www/vhosts/limoncello-collins/bootstrap/cache/compiled.php(2236): Illuminate\Foundation\Http\Kernel->renderException()
 #7 /var/www/vhosts/limoncello-collins/public/index.php(54): Illuminate\Foundation\Http\Kernel->handle()
 #8 /var/www/vhosts/limoncello-collins/public/index.php(0): {main}()
 #9 {main} 

It seems that the CodecMatcher passed into in the ConvertContentRenderer created in the exception handler, has no foundEncoder, meaning $this->codecMatcher->getEncoder() returns null. However, the ConvertContentRender always expects the CodecMatcher to have an encoder, which results in this error.

Let me know if you want any more information from me.

Thanks.

[neomerx]

Shorter way to reproduce

1) get the latest version of limoncello-collins 2) replace AuthorsController::index in app/Http/Controllers/Demo/AuthorsController.php with

public function index()
{
    throw new \Illuminate\Contracts\Validation\ValidationException(new \Illuminate\Support\MessageBag([
        'first_name' => 'First name must be provided',
        'last_name'  => 'password must be 7 characters'
    ]));
}

3) run AuthorsTest::testGetAuthors from Demo/Api/AuthorsTest.php

[neomerx]

The reason is that parsing of input parameters were removed and no encoder was matched for requested data format.

You should check input parameters in request or make sure there are no such parameters. For example

public function index()
{
    $this->checkParametersEmpty();

    ...
}

[philbates35]

Hi, thanks for your reply.

You are correct - if I call $this->checkParameters() before throwing my exception in the controller method, then my custom exception renderer works. However this breaks down if any part of my application happens to throw an exception before hitting my controller method. For example, I might check if the user has authentication as a before middleware before hitting the route, and throw a \Illuminate\Contracts\Validation\UnauthorizedException if they don't - this would happen before the controller route is hit and the parameters have been checked. Then, when I try to convert the UnauthorizedException into a response with content in my exception handler, I get the 500 and error message reported in the first post. Also, if the application throws an exception at any point before before the controller route is hit, then any custom renderers for these exceptions won't work either.

Maybe the encoder should be determined during service provider registration so that it is available throughout the entire application?

EDIT: Ultimately I'd like for any exception thrown at all to generate a response with {errors: {...}} in the body. I'd like custom renderers for specific exceptions, and finally a fallback catch-all exception renderer that will convert any exception thrown at any point into a valid JSON API response containing {errors: {...}}.

[neomerx]

It could be an enhancement to split input data check into parsing&checking headers and URL parameters. Parsing of headers is obligatory for all requests (move to middleware?) and actual check of parameters is actually optional.

[neomerx]

ServiceProvider::boot could be a good candidate as well (instead of middleware) as it needs only Request. The only downside the code will be executed even if some middleware return an error however IMO it won't have big impact on performance as headers parsing is relatively simple task.

[philbates35]

I think that makes a lot of sense.

One question - if header check fails (during ServiceProvider::boot() like you are suggesting), then an exception will be thrown using either ExceptionThrowerInterface::throwNotAcceptable() or ExceptionThrowerInterfacethrowUnsupportedMediaType(). I haven't tried, but I assume that when one of these exceptions is thrown and I try to render it using a custom exception renderer, I'll get the 500 plus Call to a member function encodeErrors() on null error - can you think of a way around that? Does trying to respond with content with the Accept Header isn't valid even make sense?

[philbates35]

Thinking about it, header checking is much more suited to route middleware than a service provider. The reason is, in my application, all URIs prefixed with /api should be treated at the JSON API end points, but all other URIs should be treated like a standard web application. I would only want header checking to be applied to my /uri routes, which I can do easily using a route middleware. If it was done in the service provider, it would check the headers against all routes, which should not happen.

[neomerx]

I tend to agree despite boot could be used middlware is a more natural choice.

[neomerx]

Headers check is now out of Controllers/API. It's now in request where headers and body could be checked.