[v4] Support creating a queryBuilder for a Model instance

[denjaland]

When we construct our APIs, we often have similar endpoints:

GET /accounts returns all accounts GET /accounts/{account} returns a specific account.

To ensure we provide an API as clean as possible to our consumers, we want to have the same ?include= and and ?fields query parameters, so in our GET /accounts/{account} endpoint controller we usually ended up doing the following:

class GetAccountController {
  public function __invoke(Account $account): AccountResource {
    return AccountResource::make(
      QueryBuilder::for(Account::class)
        ->where('id', $account->id)
        ->allowedIncludes([
          AllowedInclude::relationship('address'),
        ])
        ->first()
     );
  }
}

This works of course, but becomes a bit verbose, so what we are introducing with this pull request, is that we can also create a QueryBuilder for a model instance, allowing us to rewrite the above as:

class GetAccountController {
  public function __invoke(Account $account): AccountResource {
    return AccountResource::make(
      QueryBuilder::for($account)
        ->allowedIncludes([
          AllowedInclude::relationship('address'),
        ])
        ->first()
     );
  }
}

Just one remark: even though it was undocumented, we could already pass in the instance as a parameter for the static for() method, but it would actually return a query builder for the Model collection without filtering the actual model instance:

public static function for($subject, ?Request $request = null): static
  {
    if (is_subclass_of($subject, Model::class)) {
      $subject = $subject::query();
    }
  }

This means that this PR may impact users that rely on being able to just pass down a model instance and it returning the queryBuilder for the Model instead of a queryBuilder that filters for the concrete instance, as we change the above to this:

public static function for($subject, ?Request $request = null): static
  {
    if (is_subclass_of($subject, Model::class)) {
      if(is_object($subject)) {
        $subject = $subject::query()->where($subject->getKeyName(), $subject->getKey());
      } else {
         $subject = $subject::query();
      }
    }
    return new static($subject, $request);
  }

Hence I would personally suggest releasing this in a major version.

thanks again for considering this PR.

[timacdonald]

I like this idea and have a half baked PR to do a similar thing.

class UserController
{
    public function show(User $user)
    {
        QueryBuilder::for($user)
            ->allowedIncludes(['posts'])
            ->load();

        return UserResource::make($user);
    }
}

This was the dream API I wanted to achieve, with the main thing being that it doesn't re-query for the base model.

What is your take on that idea @denjaland?

[denjaland]

Hi @timacdonald!

I must be honest: I started the journey for this PR also with the idea of wanting to eliminate the redundant query. I'm one of those guys that is always zooming in on performance and am allergic to N+1 query issues.

But as I was working on it, I felt like things got complicated quite quickly; you almost had to have a second implementation of all the options the query builder supports. In stead of doing $query->with(), I now had to call $model->load()for instance, and what about selecting fields when the fields were already retrieved anyway?

And so I looked back a little bit... What is the responsibility of something we name QueryBuilder? Right: it builds queries and nothing more.

So my conclusion was that what I was initially aiming for (avoiding the redundant query), is not the responsibility of this package, and is something we might build on top of it, if we ever decided we needed it.

At this time, however, we decided to live with the additional query, considering that the impact will be relatively limited.
We are still avoiding the N+1 query using the package, and a 2 query issue is something we can live with for the time being. If ever we feel like it's a real issue, we would probably build something on top of the query builder.

But we definitely had the same ideas when starting this! 👍

[timacdonald]

On second look at things, I agree. Probably why I abandoned my first attempt. Thanks @denjaland

[AlexVanderbist]

Great idea! I agree that this would be a breaking change, so I'll keep this ready for a future v4 release 👍

[spatie-bot]

Dear contributor,

because this pull request seems to be inactive for quite some time now, I've automatically closed it. If you feel this pull request deserves some attention from my human colleagues feel free to reopen it.