Dingo severely messes up Laravel web routes

[zlanich]

Q	A
Bug?	yes
New Feature?	no	yes
Framework	Laravel
Framework version	5.8.36
Package version	2.4.5
PHP version	7.3

Actual Behaviour

Dingo API's LaravelServiceProvider applies the Dingo\Api\Http\Middleware\Request middleware to the entire application, rather than just the api routes. This causes severe issues in web routes. Initially, it seemed like it was working, but in the process of debugging why all my form values were getting wiped out on redirect with errors, I discovered that the view was being processed 4 times, and the form values were getting wiped out of the resulting request/session in one of those iterations. I commented out the $kernel->prependMiddleware(Request::class); line in LaravelServiceProvider and everything works perfect, and my web routes are ~4-5x faster now, so it's also causing performance issues application-wide.

Expected Behaviour

Dingo middlewares or anything route-related should not be active in non-api routes at all, and should not interfere with them.

Steps to Reproduce

I'm using specialtactics/laravel-api-boilerplate, which obviously has Dingo API in it, and somewhat unrelated, I'm using kris/laravel-form-builder in my forms. It turns out the form builder was not the issue. Reproducing is as simple as installing this boilerplate, adding some sort of web route that submits a form and redirects back to the form page with redirect()->back()->withInput();, and you'll notice that the session values, etc are there at least on initial render, but if you throw a debug statement in the resulting blade view cache file, you'll see that it hits the break point several times. Comment out $kernel->prependMiddleware(Request::class); as mentioned about, and all problems go away.

Possible Solutions

Just base all your service provider registrations on a per-route-group basis in some way. Make it only active on api routes, and/or allow the user to opt in/out of what route groups it's registered in.

I'd be glad to provide more information, but it seems the issue and general idea of why this is an issue and how to solve it is pretty clear. Let me know if you need more info!

My temporary override is as follows:

class Kernel extends HttpKernel
{
    //...

    protected function sendRequestThroughRouter($request) {
        $this->app->instance('request', $request);

        Facade::clearResolvedInstance('request');

        $this->bootstrap();

        // Override
        $this->maybeRemoveDingoApiRequestMiddleware($request);
        // End - Override

        return (new Pipeline($this->app))
            ->send($request)
            ->through($this->app->shouldSkipMiddleware() ? [] : $this->middleware)
            ->then($this->dispatchToRouter());
    }

    protected function maybeRemoveDingoApiRequestMiddleware($request) {
        if ($request->server->get('HTTP_HOST') !== config('api.domain')) {
            $dingoIndex = array_search(Request::class, $this->middleware);
            if ($dingoIndex !== false) {
                unset($this->middleware[$dingoIndex]);
                $this->middleware = array_values($this->middleware);
            }
        }
    }

    // ...
}

[bbashy]

Not having this issue myself. Didn't really understand your "steps" to reproduce either.

[zlanich]

@bbashy The steps to reproduce might be a little tricky if you're not using the boilerplate, or if you hit an edge case where it doesn't appear to be an issue on the surface, but the underlying issue is very clear:

If you're using Laravel to serve both API routes AND Web routes, the Dingo "API" lib should not be overtaking the entire router and modifying requests that aren't API-related. In my case, this made my stack unusable. In other cases, it may at very least affect performance.

Most middlewares have some sort of check to see if they should be processing a request in the first place, whereas this one seems to be affecting all requests naively. At very least, I think we should have a config like the CORS libraries that allows us to specify which routes should be handled by the Dingo request middleware. Anything that doesn't match would get passed to the rest of the normal middlewares without doing anything to the request:

$apiRoutes = [
  '/api/*'
];

If I'm not being clear enough, let me know. I will help in any way I can. Either way, the overarching issue is not just my edge case. The code itself shows that Dingo's Request Middleware doesn't take into consideration the possibility that non-API related routes may also be used in the Laravel stack simultaneously. If I'm misunderstanding part of your code, also let me know. I did however use xDebug and followed the code through line by line to find out that it is in fact affecting every single route.

Thanks!

[bbashy]

Dingo\Api\Http\Middleware\Request::handle
vendor/dingo/api/src/Http/Middleware/Request.php:111

Line 111 is where it continues onto the next middleware. It's not being caught by the middleware

return $next($request);

This is having minimal impact for me. How is it causing 4-5x slowdown?

[zlanich]

EDIT: I see now your RequestValidator is supposed to validate against the domain. It's very possible that this was not working for some reason. I definitely have my API domain configured as a separate domain in the config and otherwise. I can spin the project up again soon and see if this validator is in fact failing.

---

Your handle function forces all requests to use a Dingo Request object, then feeds it through the Dingo router. There's nothing here to short-circuit to return $next($request); if it's not an api routes request. The issue I was having took several hours to track down, and I'm not 100% certain exactly which line is causing the biggest issue, but it seems like there was some sort of multiple redirect issue or something that was causing session values to be completely stripped, and I noticed it when I threw a breakpoint in the Blade templates of my forms. The breakpoint would hit several times (when it should only hit once), and after the first hit, all session values were gone, causing all my old form values to be blank. See here:

public function handle($request, Closure $next)
    {
        try {
            if ($this->validator->validateRequest($request)) {
                //
                // ie. Put a check here for config('api.matchingRoutes') and skip to return $next($request);
                //      if the route in the request does not match. This is how laravel-cors works.
                //
                $this->app->singleton(LaravelExceptionHandler::class, function ($app) {
                    return $app[ExceptionHandler::class];
                });

                $request = $this->app->make(RequestContract::class)->createFromIlluminate($request);

                $this->events->dispatch(new RequestWasMatched($request, $this->app));

                //
                // This line always runs as long as validation passes
                //
                return $this->sendRequestThroughRouter($request);
            }
        } catch (Exception $exception) {
            $this->exception->report($exception);

            return $this->exception->handle($exception);
        }

        return $next($request);
    }

The 4-5x slowdown was caused by w/e redirect issue was happening, because it was clearly routing through more than it needed to, if not looping in some way. Literally all my problems go away if I remove the Dingo middleware, and web requests sped up significantly.

[bbashy]

OK so a few things;

The if statement in the try catch returns false for me on web routes (anything other than my API).

$this->validator->validateRequest($request);

I believe you're not setting up dingo/api properly.

I have these set in my .env. The validateRequest() method above will check the values here (as you're suggesting) against the request and skip if not an API route.

API_STANDARDS_TREE=x
API_SUBTYPE=appname
API_PREFIX=api
API_VERSION=v1
API_STRICT=true
API_DEBUG=false
API_DEFAULT_FORMAT=json

[zlanich]

So the only relevant difference in mine is that I don't have/want a prefix. FYI: My admin UI is on admin.app.test, and my api is on api.app.test. APP_URL is kind of an odd configuration at this point since we're running a subdomain for the admin UI.

APP_URL=http://app.test

API_DOMAIN=api.app.test
API_STANDARDS_TREE=x
API_SUBTYPE=Laravel
API_PREFIX=
API_VERSION=v1
API_STRICT=false
API_DEBUG=false
API_DEFAULT_FORMAT=json

Is there a reason we have to have a prefix? I didn't think it made sense to have api.app.test/api.

[bbashy]

Looking at the validateRequest in dingo, it matches both domain and prefix.

[zlanich]

Interesting. I'm honestly going to have to spin the env back up and see if/why it is indeed failing validation. Now that I know that is the validator mechanism responsible for keeping the middleware from processing the request, I can throw some breakpoints in there.

I will get back to you. Thank you for your help!

[specialtactics]

@zlanich the API boilerplate is not meant to be used in that way - it is meant to be used as a pure API project. I think for you, it would be a matter of reconfiguring the combination of Http\Kernel and Providers\RouteServiceProvider

I will close this for now, however if you find yourself having additional issues, please reply back.

[zlanich]

@specialtactics Yea, I'm aware it's not meant to be used this way specifically. Realistically, though, Laravel "is", so it's an oversight in my mind. Splitting off services into their own servers prematurely hikes resource cost, obviously. That being said, the issue isn't "supposed" to occur, given your validator, so I will play around with it a bit and see what I can do. Thanks for your help!

[specialtactics]

@zlanich I guess it's a tradeoff of using that package, up to you at the end of the day, like I said not impossible, but not the way it's meant to work.

Cost is an issue of your infrastructure, there are lots of ways to optimise it, it's a totally separate concern. From a code perspective, it is best to keep things decoupled.

[zlanich]

@specialtactics I 100% agree that it's best to keep things decoupled. In an ideal world, I would do this 100% of the time. However, when you're running a small application for a start-up and trying to minimize cost and dev-ops labor time, simply put, adding another PHP runtime server costs more money. And there's no way around it if you're running on something like Heroku where it's generally a 1 repo per app workflow. In our case, we might have an app as the primary product, with a couple admin screens for basic administration. This doesn't justify spinning up another server and maintaining another repo when Laravel is sitting right there for you to add a web route to for the time being; especially if both sides use all the same dependencies. So, I understand why the boilerplate is built like it is. It wasn't a surprise to me. It just didn't occur to me that there was a mechanism in place to keep it from running on non-api routes, and that it was just not running for some reason.

That being said, the boilerplate is great starting place for APIs in Laravel, since that space is very weak atm. Your work is appreciated.

[bbashy]

I'm using dingo/api with a large Laravel 7 app. No problems at all with performance.

[zlanich]

@bbashy I don't personally have any performance issues with Dingo in general. I think my issue is specifically that it's interfering with my web routes and creating a loop. Lucky me! lol.

[bbashy]

Well I've had no issues at all while using it. If you manage to get a minimal test app that you can reproduce it on, I'll be happy to look at it.

[zlanich]

@bbashy Much appreciated! My app is currently kind of complex, so I will have to strip it down a bit. As soon as get time to revisit it, I will know more. Thank you!