N+1 problem

[ehsan-soleimanian]

Hi @frasmage
I'm wondering that all $this->media in the following functions will face N+1 problem

public function getMedia($tags, bool $matchAll = false): Collection
    {
        if ($matchAll) {
            return $this->getMediaMatchAll($tags);
        }

        $this->rehydrateMediaIfNecessary($tags);

        return $this->media
            //exclude media not matching at least one tag
            ->filter(function (Media $media) use ($tags) {
                return in_array($media->pivot->tag, (array)$tags);
            })->keyBy(function (Media $media) {
                return $media->getKey();
            })->values();
    }

for instance when we fetch a tagged image for a $user object through $user->firstMedia('profile_image') ,it returned all media attached to the $user then walking through and find the tagged image.
I changed all $this->media to $this->media() and now everything works well.
How do you think on this matter @frasmage

[frasmage]

Hi there,

A lot of thought has gone into avoiding N+1 in the design of this library!

You are correct that just using the methods in a loop without forethought will result in N+1 queries being executed. This function is intentionally using the loaded collection as this library provides a number of methods to eager load its contents with exactly the media you require for the current process. Calling one of these methods on the query or Media collection before you iterate it will populate the media tags you require using a fixed number of database queries.

[EriBloo]

Hi.
IMO using $this->media instead of $this->media() is a better practice in Laravel. It moves responsibility on Client to avoid N+1, but more importantly it allows you to avoid it at all. Consider this code in your application:

$media = User::query()
    ->with('media')
    ->get()
    ->map(fn (User $user) => 
        $user->getMedia(['some_tag'])
    );

If $this->media() was used inside getMedia there is no way for you to fix it, additional query would always be dispatched. Furthermore $this->media N+1 can be catched with Model::shouldBeStrict() while with $this->media() it would be hidden.

Cheers! 😄