dmitrybubyakin / nova-medialibrary-field

Laravel Nova field for managing the Spatie media library
MIT License
262 stars 62 forks source link

N+1 query #111

Open zareismail opened 3 years ago

zareismail commented 3 years ago

Hi. Thanks for the amazing package.

But there is a bug that causes the N+1 query on the index view. The default mediaOnIndex callback:

 // At line 130

 function (HasMedia $resource, string $collectionName) use ($mediaOnIndex): Collection {
        return $resource->media()->where('collection_name', $collectionName)
                         ->limit($mediaOnIndex)
                         ->orderBy('order_column')
                         ->get();
  }

Do not check the loaded media relationship. This causes N+1 query on index view that can be changed like the below:

 // Uses laravel auto eager loading if not loaded

 function (HasMedia $resource, string $collectionName) use ($mediaOnIndex): Collection {
        return $resource->media->where('collection_name', $collectionName)
                         ->limit($mediaOnIndex)
                         ->sortBy('order_column');
  }
dmitrybubyakin commented 3 years ago

@zareismail Hi. You can pass a custom media resolver to mediaOnIndex https://github.com/dmitrybubyakin/nova-medialibrary-field#mediaonindex

zareismail commented 3 years ago

@zareismail Hi. You can pass a custom media resolver to mediaOnIndex https://github.com/dmitrybubyakin/nova-medialibrary-field#mediaonindex

Yeah. I know. But I think this default value is wrong and many developers will never notice it

dmitrybubyakin commented 3 years ago

@zareismail Does limit work with the eager loading?

zareismail commented 3 years ago

@zareismail Does limit work with the eager loading?

I think, no. you can change it like this:

$resource->relationLoaded('media') || $resource->load(['media' => function($query) use ($limit) {
$query->limit();
});

return $resource->media->where('collection_name', $collectionName) ...
dmitrybubyakin commented 3 years ago

@zareismail Thank you. I'll think about that.