Eager Loading a relation that calls another relation returns incorrect results

[allandantasdev]

Laravel Version

11.7.0

PHP Version

8.3.7

Database Driver & Version

PostgreSQL 15.7 and MySQL 8.0.37

Description

When eager loading a model relationships, the results differ from when they are lazy loaded.
This issue occurs whenever a Relation is called inside the definition of another Relation, but only when eager loading is used on the main one.

After investigation I realized that the cause lies in Illuminate\Database\Eloquent\Builder@eagerLoadRelation:

 // First we will "back up" the existing where conditions on the query so we can
 // add our eager constraints.
 $relation = $this->getRelation($name);
 $relation->addEagerConstraints($models);
 
 // Then we will merge the wheres that were on the
 // query back to it in order that any where conditions might be specified.
 $constraints($relation);

Which calls the Illuminate\Database\Eloquent\Builder@getRelation method:

 // We want to run a relationship query without any constraints so that we will
 // not have to remove these where clauses manually which gets really hacky
 // and error prone. We don't want constraints because we add eager ones.
 $relation = Relation::noConstraints(function () use ($name) {
     try {
         return $this->getModel()->newInstance()->$name();
     } catch (BadMethodCallException) {
         throw RelationNotFoundException::make($this->getModel(), $name);
     }
 });

Which gets to the root cause of the problem in Illuminate\Database\Eloquent\Relations\Relation@noConstraints:

$previous = static::$constraints;

 static::$constraints = false;

 // When resetting the relation where clause, we want to shift the first element
 // off of the bindings, leaving only the constraints that the developers put
 // as "extra" on the relationships, and not original relation constraints.
 try {
 return $callback();
 } finally {
 static::$constraints = $previous;
 }

The method Illuminate\Database\Eloquent\Relations\Relation@noConstraints is called during eager loading and uses a boolean attribute to manage the constraints. However, this flag is static and seems to be causing the where clauses of other relations to be omitted, leading to incorrect results.

Steps To Reproduce

1. Create the following schema

        // 0001_01_01_000000_create_users_table.php
        Schema::create('categories', function (Blueprint $table) {
            $table->bigIncrements('id');
            $table->unsignedBigInteger('user_id');
            $table->foreign('user_id')->references('id')->on('users');
            $table->text('name');
            $table->timestamps();
        });
        Schema::create('examples', function (Blueprint $table) {
            $table->bigIncrements('id');
            $table->unsignedBigInteger('category_id');
            $table->foreign('category_id')->references('id')->on('categories');
            $table->text('name');
            $table->boolean('restricted');
            $table->timestamps();
        });

2. Create the following seeder:

        $user = User::factory()->create();

        $categories = [
            Category::query()->create(['user_id' => $user->id, 'name' => 'Category 1']),
            Category::query()->create(['user_id' => $user->id, 'name' => 'Category 2']),
            Category::query()->create(['user_id' => $user->id, 'name' => 'Category 3']),
        ];

        Example::insert([
            ['category_id' => $categories[0]->id, 'name' => 'Example 1', 'restricted' => false],
            ['category_id' => $categories[1]->id, 'name' => 'Example 2', 'restricted' => true],
            ['category_id' => $categories[2]->id, 'name' => 'Example 3', 'restricted' => false],
            ['category_id' => $categories[2]->id, 'name' => 'Example 4', 'restricted' => false],
            ['category_id' => $categories[2]->id, 'name' => 'Example 5', 'restricted' => true],
        ]);

        User::factory()->create(); // another user just for demonstration

3. Create a scope in the Example model:

class Example extends Model
{
    // ...
    /**
     * The authenticated user should only have access to not restricted Examples
     * or to the examples he owns.
     */
    public function scopeHasAccess(Builder $query, ?User $user = null): Builder
    {
        return $query->where(
            fn ($query) => $query->where('restricted', false)
                ->when(
                    $user !== null,
                    fn($query) => $query->orWhereIn('category_id', $user->categories->pluck('id'))
                )
        );
    }
}

4. Add the following relations to the models:

class User extends Authenticatable
{
    // ...
    public function categories(): HasMany
    {
        return $this->hasMany(Category::class);
    }
}

class Category extends Model
{
    // ...
    public function examples(): HasMany
    {
        return $this->hasMany(Example::class)
            ->hasAccess(Auth::user());
     }
}

5. Authenticate:

    Auth::login(User::find(1));
    // Auth::login(User::find(2));

6. Fetch all categories with their respective examples

        dump('Authenticated user: '.Auth::user()->id);
        Category::get()->each(
            fn(Category $category) => dump(sprintf('- %s: %d examples', $category->name, $category->examples->count()))
        );
        
// Authenticated user: 1
// - Category 1: 1 examples"
// - Category 2: 1 examples"
// - Category 3: 3 examples"
// ----------------------------
// Authenticated user: 2"
// - Category 1: 1 examples"
// - Category 2: 0 examples"
// - Category 3: 2 examples"

7. Execute the code again but eager loading the examples relation:

        dump('Authenticated user: '.Auth::user()->id);
        Category::with('examples')->get()->each(
            fn(Category $category) => dump(sprintf('- %s: %d examples', $category->name, $category->examples->count()))
        );
        
// Authenticated user: 1
// - Category 1: 1 examples"
// - Category 2: 1 examples"
// - Category 3: 3 examples"
// ----------------------------
// Authenticated user: 2"
// - Category 1: 1 examples"
// - Category 2: 1 examples"
// - Category 3: 3 examples"

• Expected behavior:
  The fetched relations should be consistent regardless of whether they are lazy or eager loaded.

• Actual behavior:

  • Without eager loading: The user of id 2 has access to 3 examples (correct)
  • With eager loading: The user of id 2 has access to 5 examples (wrong)

[crynobone]

Hey there, thanks for reporting this issue.

We'll need more info and/or code to debug this further. Can you please create a repository with the command below, commit the code that reproduces the issue as one separate commit on the main/master branch and share the repository here?

Please make sure that you have the latest version of the Laravel installer in order to run this command. Please also make sure you have both Git & the GitHub CLI tool properly set up.

laravel new bug-report --github="--public"

Do not amend and create a separate commit with your custom changes. After you've posted the repository, we'll try to reproduce the issue.

Thanks!

[allandantasdev]

Here it is: allandantasdev/laravel-bug-report-51825@ce9741a

[allandantasdev]

Additional info:

• This is the query generated by Auth::user()->categories->pluck('id') at Example@scopeHasAccess:24 while lazy loading
  

• And is the same query generated after trying to eagerloading Category::with('examples'):
  

[github-actions]

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

[Tofandel]

I found a quick and dirty solution, in the constructor of Relation (beware that it breaks some other cases)

   public function __construct(Builder $query, Model $parent)
    {
        $this->query = $query;
        $this->parent = $parent;
        $this->related = $query->getModel();

        $this->addConstraints();

        static::$constraints = true; // This
    }

I've been working on getting a proper fix, but there doesn't seem to be a straight path forward as changing one thing breaks another one, this will likely require some debug_backtrace to fix this without breaking some test cases

[crynobone]

I believe #52461 have fixed this issue, please open a new issue if you still face the problem

[Tofandel]

Sorry, that PR doesn't address this issue at all, I was only talking about how using static causes this kind of issues

You can reopen it

        // We want to run a relationship query without any constrains so that we will
        // not have to remove these where clauses manually which gets really hacky
        // and error prone. We don't want constraints because we add eager ones.
        $relation = Relation::noConstraints(function () use ($name) {
            try {
                return $this->getModel()->newInstance()->$name();
            } catch (BadMethodCallException $e) {
                throw RelationNotFoundException::make($this->getModel(), $name);
            }
        });

@allandantasdev this makes sense because the conditions might imply columns from the model (other than the foreign key), that will not be in the query for the related model.

the solution might be to add the constraints on the collection of related models based on each model

    protected function eagerLoadRelation(array $models, $name, Closure $constraints)
    {
        // First we will "back up" the existing where conditions on the query so we can
        // add our eager constraints. Then we will merge the wheres that were on the
        // query back to it in order that any where conditions might be specified.
        $relation = $this->getRelation($name);

        $relation->addEagerConstraints($models);

        $constraints($relation);
///////////////////////////////////////////////// HERE
        // Once we have the results, we just match those back up to their parent models
        // using the relationship instance. Then we just return the finished arrays
        // of models which have been eagerly hydrated and are readied for return.
        return $relation->match(
            $relation->initRelation($models, $name),
            $relation->getEager(), $name
        );
    }

[macropay-solutions]

Sorry for the previous replies. Now we understood the real issue.
This is called

$user->categories->pluck('id')

while the

static::$constraints

is false because of the eager load of the relation in which it is called, resulting in all categories being retrieved not only the categories from that user.