n+1 Eager loading issue using $response->paginator and $availableIncludes of transformer

[Luddinus]

Hi.

I have users which have many photos.

controller

public function getUsers()
{
    $users = User::paginate(10);

    return $this->response->paginator($users, new UserTransformer);
}

UserTransformer

protected $availableIncludes = ['photos'];

public function includePhotos(User $user)
{
    $photos = $user->photos;

    return $this->collection($photos, new PhotoTransformer);
}

It runs 12 queries instead of 3? If I don't use the $response->paginator method, it works well.

What am I missing?

[kirkbushell]

Eager load it.

User::with('photos')->paginate(10);

This is a non-issue.

[Luddinus]

Yes it is.

There are dynamic includes. Works with collections but not with paginator. I don't get it.

[kirkbushell]

A dynamic include isn't an eager loaded one. Eager load it to begin with, problem solved.

[Luddinus]

The thing is, this url: users?include=photos

It "eagers load" automatically, passing User::all() and transformer to the collection method, there is no "n+1" issue.

return $this->response->collection(User::all(), new UserTransformer);
// NO n+1 issues, and it will "eager load" photos

But if I use "paginator" and User::paginate(10), it doesn't "eager load"...do you understand what I'm saying?

return $this->response->paginator(User::paginate(10), new UserTransformer);
// n+1 issue, why?

[kirkbushell]

The problem with allowing something like a URL parameter to make changes to your execution that immediately effects query results, is security, and bugs. I can easily change that to something else that doesn't exist in your database and have exceptions thrown.

It's far better to have your code control what those includes entail, and include accordingly.

I understand what you're saying, and fair enough that may be a bug - but it's an approach I would avoid with a 10-foot pole.

Use transformers to transform your output, not control the results from the database.

[lightvision]

@kirkbushell :+1:

[kirkbushell]

Also I've just realised why it's broken - it'd be looking for a users collection, and a paginator instance isn't a collection. There's a possible problem there but I still stand by what I said earlier - the approach/implementation could be improved :)

[Luddinus]

@kirkbushell I haven't seen all the code of fractal but I guess that if we use "includes", it checks in $availableIncludes.

I always did what you're saying (All the "work" in ORM) but I didn't know about Fractal, and this "includes" could be a nice approach in order to not duplicate some code in the future.

[kirkbushell]

You wouldn't duplicate code either way if it's written correctly :)

[Luddinus]

@kirkbushell I mean, the same API endpoint could have more behaviours, instead of make a lot endpoints with almost the same behaviour.

/users/with-photos (all users with photos) /users/with-albums-and-photos etc.

Probably not the best example, but you get the idea.

[kirkbushell]

Okay I'm not being very clear.

There's no reason to have other endpoints. Having url parameters that tell the API which resources the requester would like to include, is fine. The problem is having this automatically handled for you, rather than writing the code and safeguards yourself.

[jasonlewis]

I can easily change that to something else that doesn't exist in your database and have exceptions thrown.

Not exactly. The includes key only works for pre-defined includes on the transformer itself. So changing it to something else wouldn't throw an exception or include something you didn't want to be available in the response.

Dingo allows you to lazily eager load your available includes assuming they are requested in the URL query string. It's configurable, of course, as some people (like your Kirk) would prefer to keep that control elsewhere. That said, I don't think there's anything wrong with using this feature. It doesn't do anything dangerous.

As for why it's not working... Kirk was right. It currently only checks for collections. Will tweak it so that it also checks paginators. Should be doable. I'll get back to you.

[kirkbushell]

Good to know @jasonlewis. Thanks for the clarification!

[jasonlewis]

Fix pushed.

[rleger]

Still not working for me. I'm trying to eagerload nested resources such as users.companies (a user can have multiple companies). As it didn't work with paginated collections, I tried with collections but it's still not eagerloading. (using 1.0.0.beta2)

My request

GET
/interventions?include=users.companies

When using dingo :

$response = app('Dingo\Api\Http\Response\Factory');
return $response->collection($interventions, new InterventionTransformer());

It does not eagerload users.companies.

If I use fractal directly like so, it eagerloads normally :

$this->fractal->parseIncludes($includes);

$resource = new Collection($interventions, new InterventionTransformer);

$rootScope = $this->fractal->createData($resource);

return $this->respondWithArray($rootScope->toArray());

I should mention that InterventionTransformer has an includeUsers method and UsersTransformer has an includeCompanies method.

I tried to explicitly set the Fractal adapter to eagerload to make sure it's on :

$this->app['Dingo\Api\Transformer\Factory']->setAdapter(function ($app) {
    return new Dingo\Api\Transformer\Adapter\Fractal(new League\Fractal\Manager, 'include', ',', true);
});

Any idea ??

thanks !

[Luddinus]

@rleger What have you got in "includeUsers" and "includeCompanies"?

e.g this won't work

public function includeUsers($interventions)
{
   // it won't eager load
   $users = $interventions->orderBy('name')->get();

  // only will eager load $interventions->users

   return $this->collection($users, new UserTransformer);
}

I guess you would had, but did you check if you have added those includes in "availableIncludes"?

[rleger]

@Luddinus I fetch users from interventions (through a many to many relationship) like so :

public function includeUsers(Intervention $intervention)
{
    $users = $intervention->users;

    return $this->collection($users, new UserTransformer());
}

Similar stuff for includeCompanies in UsersTransformer