dingo / api

A RESTful API package for the Laravel and Lumen frameworks.
BSD 3-Clause "New" or "Revised" License
9.32k stars 1.25k forks source link

PrepareController middleware is being run too early (before user-defined middleware) since beta4? #1246

Open ngrevet opened 8 years ago

ngrevet commented 8 years ago

Since we last upgraded Dingo API, we started having strange issues on our API. I'm pretty sure I narrowed down the issue to the switch between beta3 and beta4, but I'm not 100% certain confirmed as true.

We declare route middlewares like this:

$app->routeMiddleware([
    'mw1' => Middleware1::class,
    'mw2' => Middleware2::class,
    'mw3' => Middleware3::class,
]);

And we use them like this:

$api = app(Router::class);
$api->version(
    'v1',
    function ($api) {
        $api->group(
            ['middleware' => ['mw1', 'mw2', 'mw3']],
            function ($api) {
                $api->get(
                    '/endpoint/{parameter}',
                    Controllers\EndpointController::class
                );
            }
        );
    }
);

None of the middlewares are actually applied. The middlewares themselves don't seem to be related to the issue, they are not even instantiated once. I tried switching them to application middlewares with $app->middleware() instead and the problem disappears. It's just that I can't have these run for every request, so it's not a suitable workaround. Also, it doesn't seem to work either when I switch them to a single route.

Is that something we're doing wrong or did something change recently? Seems strange that we'd be the only ones to have this issue.

rauanmayemir commented 7 years ago
// App\Http\Kernel
'api' => [
    'auth:api',
    'throttle:60,1',
    'bindings',
],

What I've found is that this code won't fire api middlewares:

// App\Providers\RouteServiceProvider
$router->group(['middleware' => 'api'], function(Router $router) {
    require base_path('routes/api.php');
});

This will:

// routes/api.php
$api = app('Dingo\Api\Routing\Router');

$api->version('v1', ['prefix' => 'api/v1', 'middleware' => 'api'], function(\Dingo\Api\Routing\Router $api) {
    // ...
})

However:

// routes/api.php
$api->version('v1', ['prefix' => 'api/v1', 'middleware' => 'api'], function(\Dingo\Api\Routing\Router $api) {
    $api->group(['middleware' => 'my_middleware'], function(\Dingo\Api\Routing\Router $api) {
        // ...
    });
})

my_middleware will be fired even if you don't pass auth, either it's overriding all previously applied middlewares or somehow bypassing them.

ngrevet commented 7 years ago

It took me quite a while to understand this one, but I finally found the reason why I had these strange issues since upgrading from beta3.

An api.controllers middleware is silently being injected by the Router::addRoute() method which calls Router::addControllerMiddlewareToRouteAction(). The problem is that this middleware is:

  1. instantiating the actual controller and kicking-off Laravel/Lumen's auto-dependency injection system that tries to resolve all of my controller needs.
  2. being injected with the highest priority and run before all other user-defined middleware.

The problem is that my user-defined middleware are actually defining some of these values that are needed by my controllers. So when Laravel/Lumen tries to resolve the controller's dependencies, they haven't been created yet.

This change fixes it, but I'm unsure if this is gonna create more issues down the line:

--- vendor/dingo/api/src/Routing/Router.php 2017-05-25 14:51:54.000000000 -0400
+++ vendor/dingo/api/src/Routing/Router.php 2016-05-25 14:51:54.000000000 -0400
@@ -364,7 +364,7 @@
      */
     protected function addControllerMiddlewareToRouteAction(array $action)
     {
-        array_unshift($action['middleware'], 'api.controllers');
+        array_push($action['middleware'], 'api.controllers');

         return $action;
     }

Any thoughts @hskrasek?