Dependency injection in server classes - problems and improvements

[X-Coder264]

Just calling JsonApiRoute::server() to register the routes produces a lot of side effects as it immediately resolves the server instance which resolves recursively all of the server dependencies which especially impacts the testing.

My use-case is that I have a client courses resource for which the index action needs to be automatically scoped so that the client can see only the courses that it has access to. The client in this case is the OAuth2 client which authenticates to the app (I'm using Passport for that). So in order to do this I'm adding a scope in the serving method of the server:

    public function serving(): void
    {
        ClientCourse::addGlobalScope($this->oAuthClientScope);
    }

And the scope code looks like this:

<?php

declare(strict_types=1);

namespace App\JsonApi\V1\ClientCourses;

use Illuminate\Contracts\Config\Repository;
use Illuminate\Database\Eloquent\Builder;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Scope;
use Laravel\Passport\Client as PassportClient;
use Laravel\Passport\Guards\TokenGuard;
use LogicException;

final class OAuthClientScope implements Scope
{
    private $request;
    private TokenGuard $tokenGuard;
    private Repository $config;

    public function __construct(
        callable $request,
        TokenGuard $tokenGuard,
        Repository $config
    ) {
        $this->request = $request;
        $this->tokenGuard = $tokenGuard;
        $this->config = $config;
    }

    public function apply(Builder $builder, Model $model)
    {
        /** @var PassportClient|null $client */
        $client = $this->tokenGuard->client(call_user_func($this->request));

        if (null === $client) {
            throw new LogicException('This should not have happened.');
        }

        // there is actually some more code here that maps the OAuth client model (oauth_clients table)
        // to the application client model (clients table), but it's not relevant for the issue here

        $builder->where('client_id', '=', $client->getKey());
    }
}

The scope instance (which gets injected into the server) has a constructor dependency on the HTTP request and on the Passport Token Guard which can tell me what OAuth client is authenticated based on the access token that is sent in the request (https://github.com/laravel/passport/blob/v10.1.2/src/Guards/TokenGuard.php#L122).

The problem here is that since the server gets resolved when the routes are registered it means that at that point in time the scope instance that is injected in the server has an empty HTTP request and for some reason the server instance is cached in the server repository (https://github.com/laravel-json-api/core/blob/v1.0.0-alpha.4/src/Core/Server/ServerRepository.php#L70-L72). That means I don't get a new server instance when I do the request in my feature test (which means that the request in the scope is empty as it was resolved before the actual request in the test was made). I was able to work around this by making the request a callable (as you can see in my scope code above) instead of directly typehinting the request class. That helped a bit as the request is now resolved only when it's actually needed instead of being resolved along with the server (and I don't consider this as a proper solution, it's more of a temporary hack), but now the problem is that the token guard depends on the OAuth2 Resource server which is supposed to be mocked by Passport in my test :

$oauthClient = PassportClientFactory::new()->create();

Passport::actingAsClient($oauthClient);

The actingAsClient essentially does app()->instance(ResourceServer::class, $mock); (https://github.com/laravel/passport/blob/v10.1.2/src/Passport.php#L395), but since the resource server was also resolved before the mocked instance was set into the container in my test I have the same problem that I get the actual non mocked resource server injected into the token guard so my test explodes as the non mocked server does not return the expected client back.

Removing the caching of the server in the server repository helps to solve this specific problem (as a new server instance gets created once my feature test does the request), but it still leaves the underlying problem unresolved and that is that the server still gets resolved on route registration and all recursive dependencies with it.

This is a problem when for example typehinting \Illuminate\Contracts\Auth\Guard in the constructor of the server and I get the default guard every time there (which is the web guard) instead of the api guard which I wanted (because the auth:api middleware that is supposed to set the guard has not been run at that point in time yet). The workaround for this is to explicitly use contextual binding for the server:

use Illuminate\Contracts\Auth\Factory as AuthFactory;

        $this->app->when(Server::class)
            ->needs(Guard::class)
            ->give(static function (Container $container): Guard {
                /** @var AuthFactory $authFactory */
                $authFactory = $container->make(AuthFactory::class);
                return $authFactory->guard('api');
            });

This works, but it only works under the assumption that the only guard that will ever be able to be used with this server is the api guard which is not a valid assumption for more complex projects where there can be multiple guards that could be used for the same API routes.

So removing the caching layer in the server repository only solves part of the problem. The other part would be making the package work in a way that that the server instance is not instantiated just for route registration. This could be solved by configuring the stuff that is needed for route registration via the config (per each server) or by making the needed data for route registration available on the server via static method(s) so that a server instance is not needed and so that the server gets resolved only when the \LaravelJsonApi\Laravel\Http\Middleware\BootJsonApi middleware runs (which can be configured in the HTTP kernel to only run after the Laravel auth middleware).

[lindyhopchris]

So you are correct that caching the server instance in the repository needs to be removed. I'll fix that.

Otherwise however I'm not going to change anything else. I've been under pressure for quite some time by developers to remove duplication of having to define the same things in multiple places. That's why the new implementation defines everything on the schema classes. Then instead of having to repeat things when defining routes, the routing implementation pulls information the schemas already know - e.g. URI fragments for the resource type and relationships; the resource ID constraint; etc - so that the developer doesn't have to redefine something the schema already knows.

The problem with your implementation is the constructor dependency injection in the server class is not appropriate. The whole point of the serving method is that the implementation guarantees that method will be invoked when the server is handling a HTTP request. The same cannot be said of the constructor. There are actually multiple places where the server is used outside of HTTP requests - e.g. routing, Artisan generator commands etc - so the constructor is not the appropriate place to be injecting a dependency that is only needed for when the server is handling an HTTP request.

Basically you should resolve the oAuth Scope from the service container within the serving method as then you know the server is definitely handling an HTTP request. As your scope is reliant on the HTTP request and associated state (such as oAuth tokens provided by the client etc), this is the correct place to instantiate it - not the server constructor.

I should point out that like you, I have a preference for using constructor dependency injection. However having worked with Laravel all these years I've realised I need to be slightly more flexible with that. The server class is one of those instances - it's used in lots of different contexts, so if you need to do something only when it is handling a HTTP request, the serving method is the place to resolve dependencies from the service contained.

[X-Coder264]

OK, I understand what you are saying. So basically what needs to be done here to solve my problems:

1. Remove the caching of the server instance in the repository
2. Make the container property of the base Server class protected instead of private so that it can be accessed in the userland server class in the serving method -> because otherwise I can't resolve anything out of the container in the serving method unless I add a new property that has the same container as the base class (https://github.com/laravel-json-api/core/blob/v1.0.0-alpha.4/src/Core/Server/Server.php#L50)
3. Pass the request instance to the serving method as a parameter so that I don't need to resolve it from the container, this is just better DX (it's technically a breaking change but since you are still in alpha releases I think that it's fine)

Please let me know if you agree with these changes and if you want me to send the PR(s) for this or if you'd like to do it yourself.

[lindyhopchris]

Actually I think the caching of the server instance can be retained, and actually is needed. The reason I added it is because the response classes can resolve the server instance multiple times. Each server instance is designed to hold its schemas so that the schemas are not constantly being reconstituted, so the server should be cached.

I'm not getting any adverse effects in my application from the server instances being cached. So I'd probably like to leave that in there for the time being, and see how your implementation works once we've made adjustments.

The approach of making the container property in the server being protected instead of private would be ok; and yes, it would make sense to pass the request to the serving hook. That will require the contract being updated too.

There is however an alternative. We could remove the serving method from the contract and use the container's call method to invoke the serving method if it exists on the server class. This would allow the serving method to type-hint dependencies. What I'm always unsure about with these magic Laravel methods is what the cost is of using the container's call method - as it would be invoked on every single request, it's potentially costly.

What do you think - use call or don't use call? I might be leaning towards not using call to potentially avoid the cost on every request.

As a side-note, I think also I'm going to be remove creating server instances via the service container. This issue has shown that really constructor DI shouldn't really be supported. It's less costly using new to create server instance, so think probably best to switch back to using that.

[X-Coder264]

I think we should assess the cost of a change in the context of a real HTTP request and how many milliseconds it adds to it versus what benefit the user gets for it.

In the example for call it's basically using \ReflectionFunctionAbstract::getParameters to get the parameters which get passed to the standard container make method (if there are any). Since Laravel already calls the make method hundreds (if not thousands) of time just to boot up the framework I think that having to resolve one or two more dependencies on each request won't have any visible impact on the performance. Besides users will be able to make the same thing if the container property becomes protected.

What is nice about the call method is that it allows contextual binding just like constructor injection (https://laravel.com/docs/8.x/queues#handle-method-dependency-injection) which is a nice feature that won't be possible to achieve with just making the container property protected.

I think that adding call is a good idea as it shouldn't hurt performance at all (independent of if the serving method will have zero or a few typehinted dependencies) so from my perspective it's a nice thing to have the option of using (especially since it's already being used in the form requests rules method so Laravel users are already used to seeing that).

But if you think that the performance hit would be too severe I think it'd be best to make some actual real world benchmarks before rejecting the idea, especially since there's a 99% likelihood that the reason a JSON API backend application is slow is due to slow DB queries, not the amount of dependencies being resolved from the service container.

[lindyhopchris]

Good points, I'll go with adding call.

In terms of performance, that's something to look at when the library is actually fully built and up and running. I.e. work out performance improvements once we know what the full package is.

I'll make those changes shortly.

[lindyhopchris]

@X-Coder264 can you update to develop and give it a go with your implementation?

I've removed constructor dependency into the server class, but am now using call for the serving hook so you can type-hint dependencies in that.

The server's $container property is also now protected.

Caching server instances is retained however, so you need to not cache any state into your server class (which shouldn't be necessary now any way).

Let me know if all's good once you've tried it.

[X-Coder264]

Thank you @lindyhopchris. I'll try it out in the next 4-8 hours and report back if everything works as expected.

[X-Coder264]

I can confirm that now everything works as expected and that my tests are green :tada: . Thank you @lindyhopchris for the time you've spent on this.

[lindyhopchris]

No problem!