Framework: Eager-loading with a limit on collection only loads for last element in collection

[jstoone]

• Laravel Version: 5.3.*
• PHP Version: 5.6 + 7.0 + 7.1
• Database Driver & Version: sqlite (3.14.0) + mysql (5.7.17 Homebrew)

Description:

There are some issues that might relate to this one, but for me they either seemed like the examples were not clear or contained domain logic that might not relate to the bug itself. See a list of such issues at the bottom.

Below I will try to explain the bug in what I see as its simplest form.

We have

• A user model
• A books model

The user has many books, but sometimes we only want 5 books, so In our User model we have the following:

/**
 * Defines HasMany relationship between the user and its books
 */
public function books()
{
    return $this->hasMany(\App\Books::class);
}

/**
 * Limit the relationship to only include 5 results
 */
public function onlyFiveBooks()
{
    return $this->books()->take(5);
}

• We have two users
• Both users have 10 books

What I expected

// Given
$users = \App\User::all();

// Act
$users->load('onlyFiveBooks');

// Assert we have two users
$users->count() == 2 // true

// Assert first user has 5 loaded books
$users->first()->getRelation('books')->count() == 5 // false
// Assert last user has 5 loaded books
$users->last()->getRelation('books')->count() == 5 // true

So I expected both users to have 5 loaded books, given the relationship defined above.

What have I tried?

// FAIL 1: Using the previously defined `books()` relationship, same as above
\App\User::with('onlyFiveBooks')->get();

// FAIL 2: Still only loads 5 books to the last of the two users
\App\User::with(['books' => function($query) {
    $query->take(5);
}])->get();

// FAIL 3: Still only loads 5 books to the last of the two users
$users = \App\User::all();
$users->load(['books' => function($query) {
    $query->take(5);
}]);

// SUCCESS: Being explicit (and maybe redundant) does produce the correct outcome
$users = \App\User::all();
$users->each(function($user) {
    $user->load('onlyFiveBooks');
});
// Assert first user has 5 loaded books
$users->first()->getRelation('books')->count() == 5 // true
// Assert last user has 5 loaded books
$users->last()->getRelation('books')->count() == 5 // true

In the wild

I've setup a project with a failing test to illustrate this bug.

git clone https://github.com/jstoone/laravel \
  -b bug-in-a-limited-eager-load-example \
  jstoone-laravel-example

cd jstoone-laravel-example

composer install

cp .env.example .env

phpunit

Somewhat similar issues

• https://github.com/laravel/framework/issues/17845
• https://github.com/laravel/framework/issues/17328

[unstoppablecarl]

\App\User::with(['books' => function($query) {
    $query->take(5);
}])->get();

// and

$users->load(['books' => function($query) {
    $query->take(5);
}]);

Generate this query:

select * from `books` where `books`.`user_id` in (1, 2) limit 5

Which makes sense. As written you are loading all user books and limiting the results of all book results to 5. I'm not sure there is a way to write a performant query that eager loads and limits results per user.

Typically you use this technique to filter the eager loaded results:

$users->load(['books' => function($query) {
    $query->where('published', true);
}]);

$users = \App\User::all();
$users->each(function($user) {
    $user->load('onlyFiveBooks');
});

Generates these queries:

select * from `books` where `books`.`user_id` in (1) limit 3
select * from `books` where `books`.`user_id` in (2) limit 3

[themsaid]

As @unstoppablecarl described, the generated query while eager loading looks like:

select * from `books` where `books`.`user_id` in (1, 2)

In order for you to limit the number of retrieved models per parent model you need to execute a query per parent model, which eliminates the need for eager loading.

If you don't mind having multiple queries like this then you can simply use something like:

$users->each(function($user) {
    $user->load('onlyFiveBooks');
});

Moreover I believe you can use a sql query to achieve what you want, but I think it's going to be a bit complicated.

Anyways I'm closing this issue since it's not really a bug in the framework, please let me know if you need any further clarification though.

[amjadkhan896]

@themsaid. Yes this works. I am facing the same problem. But the issue with your solution is for each item it fires 2 queries to the server. And it means for 10 items it will fire 20 extra queries. and so on.
Any other solution to fix this

[dimitvassilev]

What comes to mind is adding a bool column like 'in_first' (or 'in_first_five' if it is guaranteed that this number would not change) to the books table and set it to 'true' for the first n(=5 in this case) books.
Then

public function onlyFiveBooks()
{
    return $this->books()->where( 'in_first_five', true);
}

[mpyw]

UNION ALL solution (PHP 7.1)

[Laravel] Eager Loading + LIMIT を UNION ALL で実現する - Qiita

<?php

declare(strict_types=1);

namespace App\Providers;

use Illuminate\Database\Eloquent\Collection;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Relations\Relation;
use Illuminate\Support\ServiceProvider;
use Illuminate\Support\Str;

class MacroServiceProvider extends ServiceProvider
{
    public function boot(): void
    {
        Collection::macro('loadUsingLimit', function ($relations): Collection {
            /** @var Collection $collection */
            $collection = $this;
            return MacroServiceProvider::loadUsingLimit($collection, is_string($relations) ? func_get_args() : $relations);
        });
    }

    /**
     * @param  Collection $collection
     * @param  array      $relations
     * @return Collection
     */
    public static function loadUsingLimit(Collection $collection, array $relations): Collection
    {
        foreach ($relations as $name => $constraints) {

            if (is_int($name)) {
                $name = $constraints;

                if (Str::contains($name, ':')) {
                    $name = explode(':', 'name', 2)[0];
                    $constraints = function (Relation $query) use ($name) {
                        $query->select(explode(',', explode(':', $name, 3)[1]));
                    };
                } else {
                    $constraints = function () {};
                }
            }

            /** @var Relation $relation */
            $relation = $collection
                ->map(function (Model $model) use ($name) {
                    return $model->{$name}();
                })
                ->each(function (Relation $relation) use ($constraints) {
                    $constraints($relation);
                })
                ->reduce(function (?Relation $carry, Relation $query) {
                    return $carry ? $carry->unionAll($query) : $query;
                });

            $relation->match(
                $relation->initRelation($collection->all(), $name),
                $relation->get(), $name
            );
        }

        return $collection;
    }
}

class Book extends Model
{
    public function chapters(): HasMany
    {
        return $this->hasMany(Chapter::class);
    }
}

class Chapter extends Model { }

$books = Book::all()->loadUsingLimit([
    'chapters' => function (HasMany $query) {
        $query->orderBy('id')->limit(5);
    },
]);

[chaddwick25]

Does the solution that @mpyw provided solved the original problem?
/*
FAIL 2: Still only loads 5 books to the last of the two users
\App\User::with(['books' => function($query) {
$query->take(5);
}])->get();
*/

[jstoone]

@chaddwick25 looking back I see that I had a wrong approach to the problem itself. And I've realized that often times, when working with Eloquent, it's very beneficial to "flip" the query so that I would be chunking up books in groups of 5, and then eager-load the User.

Since I wanted to keep the issue as small an concise as possible, and as well use a simplified scenario my "problem" lost context.
My actual problem was not within the logic of "eager loading only X amount of books pr. user", but I could have prevented the logic flaw entirely by making my frontend query for a (explicitly) paginated list of books, where they all have eager-loaded the author/user.

Making this feature-change would also expose some hidden logic (only load 5 books pr. user) and make the logic way more apparent by explicitly paginating books and utilizing a "straight forward" eager-load to flip the logic and effectively solve my problem.

I hope this can put an end to this thread. :v:

[raojeet]

$result = $query->with(['task' => function($query) {
                                    $query->with([config('constants.DISBURSEMENT'), config('constants.EXTERNAL'), config('constants.INTERNAL')])
                                    ->orderBy('budget_phase_tasks.task_order', 'asc');
                                }])->leftJoin('budget_phase_locks', function($join) use($userId) {
                                $join->on('budget_phase_locks.phase_id', '=', 'budget_phases.id')
                                ->on('budget_phase_locks.user_id', '=', DB::raw($userId))
                                ->on('budget_phase_locks.budget_id', '=', 'budget_phases.budget_id');
                            })
                            ->Select('budget_phases.*', DB::raw('budget_phase_locks.user_id as in_used_by'))
                            ->get()->map(function($task) {
                        $task->task->map(function($data) {
                            $data->setRelation('disbursement', $data->disbursement->take(config('constants.INNER_LIMIT')))
                            ->setRelation('external', $data->external->take(config('constants.INNER_LIMIT')))
                            ->setRelation('internal', $data->internal->take(config('constants.INNER_LIMIT')));
                            return $data;
                        });
                        return $task;
                    }); 

use setRelation() with colleaction map()
function it limit nested eagr loading

[mpyw]

@chaddwick25 It will work

$users = User::all()->loadUsingLimit('onlyFiveChapters');

[aminkhoshzahmat]

worked for me! :+1:

[meicuihui]

Thank you, perfect solution to my problem. My question is to check the list of articles and the comments of each article, but an article can have a lot of comments. I only need to display 2 articles.

[staudenmeir]

I released the rejected PR as a package: https://github.com/staudenmeir/eloquent-eager-limit

[mpyw]

@standaniels +1 for grammar level approach